-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Resharding - Adding shards to an existing index #121082
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Resharding - Adding shards to an existing index #121082
Conversation
|
I have not tested this out yet. The main approach is in MetadataAutoshardIndexService.java |
…OC_addShards Refresh to latest
…/elasticsearch into 01172025/AutoShardPOC_addShards git pull
bcully
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this operation isn't an actual safe reshard yet - no data is relocated or anything. But it might be nice to include a starter test that sets up, say, a single shard index, then reshards it and then submits some indexing and search operations and confirms that they are handled correctly (all data indexed and returned on search) and that the data spans shards?
...rg/elasticsearch/action/admin/indices/autoshard/AutoshardIndexClusterStateUpdateRequest.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| public String index() { | ||
| return index; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we generally refer to indices by name internally? I had imagined that they would come in that way at the client side because that's the client-friendly way to refer to one, but that we might translate them to something less ambiguous like the uuid internally so we wouldn't have to think about things like ABA races.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I have seen, we use the name and the Index data structure quite interchangeably in the code. Not sure if there are some guidelines around which of the two to pass around internally.
| } | ||
|
|
||
| public String cause() { | ||
| return cause; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is some standard request component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, I think a request can contain whatever we think we need to put in the request. I was modeling this after CreateIndexClusterStateUpdateRequest and a cause field seemed useful for a re-sharding request. But really this is as bare bones a request as possible for re-sharding, we might need to add more stuff to this.
| waitForActiveShardsTimeout, | ||
| delegate.map(shardsAcknowledged -> { | ||
| if (shardsAcknowledged == false) { | ||
| logger.debug( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be good for this to be higher than debug, it's I guess not going to be spammy.
| }) | ||
| ); | ||
| } else { | ||
| logger.trace("index creation not acknowledged for [{}]", request); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When does this happen? Is it an error condition?
|
|
||
| Settings.Builder settingsBuilder = Settings.builder().put(sourceMetadata.getSettings()); | ||
| settingsBuilder.remove(IndexMetadata.SETTING_NUMBER_OF_SHARDS); | ||
| settingsBuilder.put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, sourceNumShards * 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it might be for later, but seems like it might be nice for the new number of shards to be a request parameter.
…OC_addShards Move TransportStatelessAutoshardAction to stateless.java
…OC_addShards Refresh
…OC_addShards Update to latest
…OC_addShards Update branch
…/elasticsearch into 01172025/AutoShardPOC_addShards pull
server/src/main/java/org/elasticsearch/cluster/routing/UnassignedInfo.java
Outdated
Show resolved
Hide resolved
…OC_addShards Refresh to latest
…/elasticsearch into 01172025/AutoShardPOC_addShards git pull
…OC_addShards Latest
server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java
Show resolved
Hide resolved
…OC_addShards Refresh branch
…OC_addShards refresh to latest
henningandersen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good though I am in doubt about the routing-num-shards part.
| settings = Settings.builder().put(settings).put(SETTING_NUMBER_OF_SHARDS, shardCount).build(); | ||
| var newPrimaryTerms = new long[shardCount]; | ||
| System.arraycopy(primaryTerms, 0, newPrimaryTerms, 0, this.primaryTerms.length); | ||
| primaryTerms = newPrimaryTerms; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should fill the remaining part of the primary terms array with UNASSIGNED_PRIMARY_TERM, just like in initializePrimaryTerms and leave it to the client to fill those in.
| var newPrimaryTerms = new long[shardCount]; | ||
| System.arraycopy(primaryTerms, 0, newPrimaryTerms, 0, this.primaryTerms.length); | ||
| primaryTerms = newPrimaryTerms; | ||
| routingNumShards = shardCount; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about this bit. I think we discussed it previously but do not recall the conclusion. When I look at MetadataCreateIndexService.calculateNumRoutingShards, it looks somewhat different to this.
| } | ||
| } else if (reason.equals(Reason.RESHARD_ADDED) | ||
| && out.getTransportVersion().before(TransportVersions.UNASSIGENEDINFO_RESHARD_ADDED)) { | ||
| out.writeByte((byte) Reason.FORCED_EMPTY_PRIMARY.ordinal()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we assert false here? I think we want this to never happen, i.e., do resharding in mixed clusters. I'd probably also prefer throwing over sending a more or less random reason here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok sure. But then should we also assert in the Unassigned constructor ?
Tim-Brooks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing) |
This POC attempts to double the number of shards of an existing index.
This POC attempts to double the number of shards of an existing index.
This POC attempts to double the number of shards of an existing index.