Skip to content
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

Remove version in ShardRouting (now obsolete) #16243

Merged
merged 1 commit into from
Feb 4, 2016

Conversation

ywelsch
Copy link
Contributor

@ywelsch ywelsch commented Jan 26, 2016

With the changes in #14739, allocation ids are now being used to select primary shards instead of version information stored with the shard state on disk. As this was the only use case so far to carry version information in shard routing, we can safely remove it.

@@ -337,7 +337,7 @@ public void updateRoutingEntry(final ShardRouting newRouting, final boolean pers
}
// if its the same routing except for some metadata info, return
if (currentRouting.equalsIgnoringMetaData(newRouting)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

does this check make any sense now that we have no versions?

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 replaced the check by an equals (no need to ignore metadata now). The check is still needed as we only want to call the shardRoutingChanged listener later if something changed.

@bleskes
Copy link
Contributor

bleskes commented Jan 27, 2016

This is great. Left some minor comments here and there...

@ywelsch
Copy link
Contributor Author

ywelsch commented Jan 27, 2016

@bleskes pushed a new set of changes

@ywelsch ywelsch force-pushed the fix/remove-shardrouting-version branch 2 times, most recently from 1171f07 to 6646f1e Compare February 2, 2016 14:12
@ywelsch
Copy link
Contributor Author

ywelsch commented Feb 2, 2016

@bleskes I rebased on current master (there were lots of conflicts due to the changes in Index class)

@@ -132,10 +132,10 @@ public DiscoveryNode getNode() {
}

/**
* Version of the store
* Version of the store for pre-3.0 shards that have not yet been active
*/
public long getVersion() {
Copy link
Contributor

Choose a reason for hiding this comment

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

rename the method as well?

@bleskes
Copy link
Contributor

bleskes commented Feb 3, 2016

Thanks @ywelsch . Left some extremely minor comments. Feel free to address them and push when ready (or ping me if you feel another cycle is needed).

@ywelsch ywelsch force-pushed the fix/remove-shardrouting-version branch 3 times, most recently from ca8c1d9 to bc49905 Compare February 4, 2016 14:24
@ywelsch ywelsch force-pushed the fix/remove-shardrouting-version branch from bc49905 to 4937531 Compare February 4, 2016 14:51
ywelsch pushed a commit that referenced this pull request Feb 4, 2016
@ywelsch ywelsch merged commit 1550758 into elastic:master Feb 4, 2016
writeReason = "freshly started, version [" + newRouting.version() + "]";
} else if (currentRouting.version() < newRouting.version()) {
writeReason = "version changed from [" + currentRouting.version() + "] to [" + newRouting.version() + "]";
writeReason = "freshly started, allocation id [" + newRouting.allocationId() + "]";
Copy link
Contributor

Choose a reason for hiding this comment

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

fyi - this gives you double [[]]

Mpdreamz added a commit to Mpdreamz/elasticsearch that referenced this pull request Apr 25, 2016
Mpdreamz added a commit that referenced this pull request Apr 26, 2016
@lcawl lcawl added :Distributed Indexing/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. and removed :Allocation labels Feb 13, 2018
@clintongormley clintongormley added :Distributed Coordination/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) and removed :Distributed Indexing/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. labels Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Coordination/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) >enhancement v5.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants