Skip to content

Conversation

bcully
Copy link
Contributor

@bcully bcully commented Oct 6, 2025

Serialization of this field is determined by the serialization of the request in which it is embedded, so it cannot sensibly serialize itself.

Serialization of this field is determined by the serialization of the request
in which it is embedded, so it cannot sensibly serialize itself.
@bcully bcully requested review from lkts, ankikuma and Copilot October 6, 2025 19:41
@bcully bcully self-assigned this Oct 6, 2025
@bcully bcully added >non-issue :Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. v9.3.0 labels Oct 6, 2025
@elasticsearchmachine elasticsearchmachine added the Team:Distributed Indexing Meta label for Distributed Indexing team label Oct 6, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing)

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the serialization logic for SplitShardCountSummary by moving the serialization responsibility from the summary class itself to the containing ReplicationRequest class. The change simplifies the design by making serialization dependent on the request's transport version rather than having the summary handle its own serialization.

Key changes:

  • Remove Writeable interface implementation from SplitShardCountSummary
  • Add static factory method and integer conversion methods to SplitShardCountSummary
  • Move transport version constants and serialization logic to ReplicationRequest

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
SplitShardCountSummary.java Removes serialization capabilities and adds factory/conversion methods
ReplicationRequest.java Takes over serialization responsibility with transport version handling

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

}

// visible for testing
SplitShardCountSummary(int shardCountSummary) {
Copy link
Preview

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The constructor visibility was changed from package-private with a comment 'visible for testing' to package-private without comment. Consider adding back the visibility comment or making it private since it's now only used internally via the fromInt() factory method.

Suggested change
SplitShardCountSummary(int shardCountSummary) {
private SplitShardCountSummary(int shardCountSummary) {

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't mean to remove the comment. Thanks copilot!

return shardCountSummary;
}

private final int shardCountSummary;
Copy link
Preview

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The field declaration was moved after the method definitions. Consider keeping field declarations at the top of the class for better readability and consistency with Java conventions.

Copilot uses AI. Check for mistakes.

@bcully bcully merged commit 711392c into elastic:main Oct 6, 2025
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. >non-issue Team:Distributed Indexing Meta label for Distributed Indexing team v9.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants