-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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 _primary and _replica shard preferences #26791
Conversation
The shard preference `_primary`, `_replica` and its variants were useful for the asynchronous replication. However, with the current impl, they are no longer useful and should be removed. Closes elastic#26335
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 left some comments. We will also separately need to deprecate these preferences in the 6.x branch and this PR needs a breaking changes note in the migration docs for 7.0.0.
String[] removedPreferences = {"_primary", "_primary_first", "_replica", "_replica_first"}; | ||
for (String pref : removedPreferences) { | ||
try{ | ||
operationRouting.searchShards(clusterState, new String[]{"test"}, null, pref); |
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.
This can be replaced with expectThrows
.
@@ -210,7 +211,10 @@ public void testCorruptTranslogTruncation() throws Exception { | |||
logger.info("--> starting the replica node to test recovery"); | |||
internalCluster().startNode(); | |||
ensureGreen("test"); | |||
assertHitCount(client().prepareSearch("test").setPreference("_replica").setQuery(matchAllQuery()).get(), numDocsToKeep); | |||
for(String node : internalCluster().nodesInclude("test")){ |
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 you add extra spacing like:
for( -> for (
)){ -> )) {
Your IDE can help with this.
@@ -308,7 +312,9 @@ public void testCorruptTranslogTruncationOfReplica() throws Exception { | |||
logger.info("--> starting the replica node to test recovery"); | |||
internalCluster().startNode(); | |||
ensureGreen("test"); | |||
assertHitCount(client().prepareSearch("test").setPreference("_replica").setQuery(matchAllQuery()).get(), totalDocs); | |||
for(String node : internalCluster().nodesInclude("test")){ |
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.
Extra spacing here:
for( -> for (
)){ -> )) {
@@ -308,7 +312,9 @@ public void testCorruptTranslogTruncationOfReplica() throws Exception { | |||
logger.info("--> starting the replica node to test recovery"); | |||
internalCluster().startNode(); | |||
ensureGreen("test"); | |||
assertHitCount(client().prepareSearch("test").setPreference("_replica").setQuery(matchAllQuery()).get(), totalDocs); | |||
for(String node : internalCluster().nodesInclude("test")){ | |||
assertHitCount(client().prepareSearch("test").setPreference("_only_nodes:"+node).setQuery(matchAllQuery()).get(), totalDocs); |
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.
Spacing here:
:"+node -> :" + node
SearchResponse searchResponse = client().prepareSearch("test").setQuery(QueryBuilders.termQuery("field", "test")).execute().actionGet(); | ||
assertHitCount(searchResponse, 1); | ||
|
||
for(String node : internalCluster().nodesInclude("test")){ |
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.
Spacing here:
for( -> for (
)){ -> )) {
assertEquals(foundHits1, foundHits2); | ||
// TODO: do something more with the replicas! index? | ||
Set<Integer> counts = new HashSet<>(); | ||
for(String node : dataNodes(index, client())){ |
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.
Spacing here:
for( -> for (
)){ -> )) {
Set<Integer> counts = new HashSet<>(); | ||
for(String node : dataNodes(index, client())){ | ||
Map<String, Object> responseBody = toMap(client().performRequest("GET", "/" + index + "/_search", | ||
Collections.singletonMap("preference", "_only_nodes:"+node))); |
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.
Spacing here:
:"+node -> :" + node
assertCount(index, "_primary", numDocs); | ||
assertCount(index, "_replica", numDocs); | ||
|
||
for(Shard shard : buildShards(index, nodes, newNodeClient)){ |
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.
Spacing:
for( -> for (
)){ -> )) {
@jasontedor, I have addressed your comments. Please have a look. Thank you. |
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.
Two more nits and it looks good.
@@ -11,3 +11,6 @@ cluster names may no longer contain `:`. | |||
The default value for the `wait_for_active_shards` parameter of the open index API | |||
is changed from 0 to 1, which means that the command will now by default wait for all | |||
primary shards of the opened index to be allocated. | |||
|
|||
==== shard preferences `_primary`, `_primary_first`, `_replica`, and `_replica_first` are removed |
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.
Nit: capitalize shard
here as it's a section title.
@@ -67,7 +66,7 @@ public void testStopOneNodePreferenceWithRedState() throws InterruptedException, | |||
refresh(); | |||
internalCluster().stopRandomDataNode(); | |||
client().admin().cluster().prepareHealth().setWaitForStatus(ClusterHealthStatus.RED).execute().actionGet(); | |||
String[] preferences = new String[] {"_primary", "_local", "_primary_first", "_prefer_nodes:somenode", "_prefer_nodes:server2", "_prefer_nodes:somenode,server2"}; | |||
String[] preferences = new String[] {"_local", "_prefer_nodes:somenode", "_prefer_nodes:server2", "_prefer_nodes:somenode,server2"}; |
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.
Nit: [] {
-> []{
.
@dnhatn Please add an area label, a type label, and version labels to this PR. |
@dnhatn I don’t think this should have the deprecation label. |
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.
Thanks @jasontedor. |
* master: update Lucene version for 6.0-RC2 version Calculate and cache result when advanceExact is called (elastic#26920) Test query builder bwc against previous supported versions instead of just the current version. Set minimum_master_nodes on rolling-upgrade test (elastic#26911) Return List instead of an array from settings (elastic#26903) remove _primary and _replica shard preferences (elastic#26791) fixing typo in datehistogram-aggregation.asciidoc (elastic#26924) [API] Added the `terminate_after` parameter to the REST spec for "Count" API Setup debug logging for qa.full-cluster-restart Enable BWC testing against other remotes
the only nodes preference was used as a replacement of `_primary` which was removed. Sadly, it's not the same as we also check that it makes sense - i.e., that the given node has a shard copy. Since the test uses indices with >1 shards, the primaries may be spread to multiple nodes. Using one (like it currently does) will fail for some primaries. Using all will probably end up hitting all nodes. This commit removed the `_only_nodes` usage in favor a simple search Relates to #26791
* master: (22 commits) Allow only a fixed-size receive predictor (elastic#26165) Add Homebrew instructions to getting started ingest: Fix bug that prevent date_index_name processor from accepting timestamps specified as a json number Scripting: Fix expressions to temporarily support filter scripts (elastic#26824) Docs: Add note to contributing docs warning against tool based refactoring (elastic#26936) Fix thread context handling of headers overriding (elastic#26068) SearchWhileCreatingIndexIT: remove usage of _only_nodes update Lucene version for 6.0-RC2 version Calculate and cache result when advanceExact is called (elastic#26920) Test query builder bwc against previous supported versions instead of just the current version. Set minimum_master_nodes on rolling-upgrade test (elastic#26911) Return List instead of an array from settings (elastic#26903) remove _primary and _replica shard preferences (elastic#26791) fixing typo in datehistogram-aggregation.asciidoc (elastic#26924) [API] Added the `terminate_after` parameter to the REST spec for "Count" API Setup debug logging for qa.full-cluster-restart Enable BWC testing against other remotes Use LF line endings in Painless generated files (elastic#26822) [DOCS] Added info about snapshotting your data before an upgrade. Add documentation about disabling `_field_names`. (elastic#26813) ...
Based on the PR message, I assume that the replication has been asynchronous at some previous version. Is there a specific version or PR that the replication model has been changed in? |
Today it is unclear what guarantees are offered by the search preference feature, and we claim a guarantee that is stronger than what we really offer: > A custom value will be used to guarantee that the same shards will be used > for the same custom value. This commit clarifies this documentation and explains more clearly why `_primary`, `_replica`, etc. are deprecated in `6.x` and removed in `master`. Relates elastic#31929 elastic#26335 elastic#26791.
Today it is unclear what guarantees are offered by the search preference feature, and we claim a guarantee that is stronger than what we really offer: > A custom value will be used to guarantee that the same shards will be used > for the same custom value. This commit clarifies this documentation and explains more clearly why `_primary`, `_replica`, etc. are deprecated in `6.x` and removed in `master`. Relates #31929 #26335 #26791.
Today it is unclear what guarantees are offered by the search preference feature, and we claim a guarantee that is stronger than what we really offer: > A custom value will be used to guarantee that the same shards will be used > for the same custom value. This commit clarifies this documentation and explains more clearly why `_primary`, `_replica`, etc. are deprecated in `6.x` and removed in `master`. Relates #31929 #26335 #26791.
Today it is unclear what guarantees are offered by the search preference feature, and we claim a guarantee that is stronger than what we really offer: > A custom value will be used to guarantee that the same shards will be used > for the same custom value. This commit clarifies this documentation and explains more clearly why `_primary`, `_replica`, etc. are deprecated in `6.x` and removed in `master`. Relates #31929 #26335 #26791.
Today it is unclear what guarantees are offered by the search preference feature, and we claim a guarantee that is stronger than what we really offer: > A custom value will be used to guarantee that the same shards will be used > for the same custom value. This commit clarifies this documentation and explains more clearly why `_primary`, `_replica`, etc. are deprecated in `6.x` and removed in `master`. Relates #31929 #26335 #26791.
Today the `?preference=custom_string_value` search preference will only change its choice of a shard copy if something changes the `IndexShardRoutingTable` for that specific shard. Users can use this behaviour to route searches to a consistent set of shard copies, which means they can reliably hit copies with hot caches, and use the other copies only for redundancy in case of failure. However we do not assert this property anywhere, so we might break it in future. This commit adds a test that shows that searches are routed consistently even if other indices are created/rebalanced/deleted. Relates https://discuss.elastic.co/t/176598, elastic#41115, elastic#26791
Today the `?preference=custom_string_value` search preference will only change its choice of a shard copy if something changes the `IndexShardRoutingTable` for that specific shard. Users can use this behaviour to route searches to a consistent set of shard copies, which means they can reliably hit copies with hot caches, and use the other copies only for redundancy in case of failure. However we do not assert this property anywhere, so we might break it in future. This commit adds a test that shows that searches are routed consistently even if other indices are created/rebalanced/deleted. Relates https://discuss.elastic.co/t/176598, #41115, #26791
Today the `?preference=custom_string_value` search preference will only change its choice of a shard copy if something changes the `IndexShardRoutingTable` for that specific shard. Users can use this behaviour to route searches to a consistent set of shard copies, which means they can reliably hit copies with hot caches, and use the other copies only for redundancy in case of failure. However we do not assert this property anywhere, so we might break it in future. This commit adds a test that shows that searches are routed consistently even if other indices are created/rebalanced/deleted. Relates https://discuss.elastic.co/t/176598, #41115, #26791
Today the `?preference=custom_string_value` search preference will only change its choice of a shard copy if something changes the `IndexShardRoutingTable` for that specific shard. Users can use this behaviour to route searches to a consistent set of shard copies, which means they can reliably hit copies with hot caches, and use the other copies only for redundancy in case of failure. However we do not assert this property anywhere, so we might break it in future. This commit adds a test that shows that searches are routed consistently even if other indices are created/rebalanced/deleted. Relates https://discuss.elastic.co/t/176598, elastic#41115, elastic#26791
The shard preference
_primary
,_replica
and its variants were usefulfor the asynchronous replication. However, with the current impl, they
are no longer useful and should be removed.
Closes #26335