Skip to content

Remove version in ShardRouting (now obsolete)#16243

Merged
ywelsch merged 1 commit intoelastic:masterfrom
ywelsch:fix/remove-shardrouting-version
Feb 4, 2016
Merged

Remove version in ShardRouting (now obsolete)#16243
ywelsch merged 1 commit intoelastic:masterfrom
ywelsch:fix/remove-shardrouting-version

Conversation

@ywelsch
Copy link
Copy Markdown
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.

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

bleskes commented Jan 27, 2016

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

@ywelsch
Copy link
Copy Markdown
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
Copy Markdown
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)

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 [[]]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Distributed/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