Skip to content
Merged
Changes from 12 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2001,12 +2001,8 @@ public Builder numberOfShards(int numberOfShards) {
* The new shard count must be a multiple of the original shardcount.
* We do not support shrinking the shard count.
* @param shardCount updated shardCount
*
* TODO: Check if this.version needs to be incremented
*/
public Builder reshardAddShards(int shardCount) {
// Assert routingNumShards is null ?
// Assert numberOfShards > 0
public Builder reshardAddShards(int shardCount, IndexMetadata sourceMetadata) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing the API this way makes it possible to accidentally supply the wrong metadata to the method, and we know we've already provided the right metadata in the builder's constructor.

I think the reason you're passing it in is that getIndexNumberOfRoutingShards wants a metadata object but we've already decomposed it into parts in the constructor. I think it's probably fine to just hold on to a reference to the whole thing for the life of the builder (i.e., this.indexMetadata = indexMetadata in the constructor or something), or to change the interface to getIndexNumberOfRoutingShards, which only has a handful of users that mostly pass in null. Maybe the first option is the simplest?

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 agree, I didn't like passing in the sourceMetadata either. But I don't know if I want to hold onto a reference to the whole thing because it looks like we would end up with some kind of recursion in toXContent() wouldn't we ?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, I hadn't noticed that when IndexMetadata goes over the wire it passes through the Builder interface.

Looking at MetadataCreateIndexService::getIndexNumberOfRoutingShards the only thing it actually uses sourceMetadata for is to call getNumberOfShards on it if it exists. So one option to avoid passing in this essentially redundant sourceMetadata field would be to refactor getIndexNumberOfRoutingShards a bit to have an inner method that just takes routingNumShards or 0 if metadata is null, which you could call directly from here, and then make the existing getIndexNumberOfRoutingShards(Settings indexSettings, @Nullable IndexMetadata sourceMetadata) just be something like return getIndexNumberOfRoutingShards(settings, sourceMetadata == null ? 0 : sourceMetadata.getRoutingNumShards()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes sure, I refactored the code.

if (shardCount % numberOfShards() != 0) {
throw new IllegalArgumentException(
"New shard count ["
Expand All @@ -2019,13 +2015,12 @@ public Builder reshardAddShards(int shardCount) {
+ "]"
);
}
IndexVersion indexVersionCreated = indexCreatedVersion(settings);
settings = Settings.builder().put(settings).put(SETTING_NUMBER_OF_SHARDS, shardCount).build();
var newPrimaryTerms = new long[shardCount];
Arrays.fill(newPrimaryTerms, this.primaryTerms.length, newPrimaryTerms.length, SequenceNumbers.UNASSIGNED_PRIMARY_TERM);
System.arraycopy(primaryTerms, 0, newPrimaryTerms, 0, this.primaryTerms.length);
primaryTerms = newPrimaryTerms;
routingNumShards = MetadataCreateIndexService.calculateNumRoutingShards(shardCount, indexVersionCreated);
routingNumShards = MetadataCreateIndexService.getIndexNumberOfRoutingShards(settings, sourceMetadata);
return this;
}

Expand Down