Enable acked indexing #17038
Enable acked indexing #17038
Conversation
… be shut down already and will an exception
* master: (350 commits) Note to configuration docs on number of threads Reduce maximum number of threads in boostrap check Limit generic thread pool Remove NodeService injection to Discovery Prevent closing index during snapshot restore [TEST] Fix newline issue in PluginCliTests on Windows ParseFieldMatcher should log when using deprecated settings. #16988 fix checkstyle error Add test for the index_options on a keyword field. #16990 Analysis : Allow string explain param in JSON Analysis : Allow string explain param in JSON fix typo Remove SNAPSHOT from versions in plugin descriptors Add support for alpha versions Enable unmap hack for java 9 Simplify mock scripts Adding `time_zone` parameter to daterange-aggregation docs Adding tests for `time_zone` parameter for date range aggregation Added ingest info to node info API, which contains a list of available processors. Remove bw compat from size mapper ...
This commit adds a guard against an old cluster state that arrives out of order from the last seen cluster state from the current master from polluting the pending cluster states queue. Without this guard, such a state can end up stuck in the pending states queue.
| @@ -820,24 +821,64 @@ void finishBecauseUnavailable(ShardId shardId, String message) { | |||
| } | |||
| } | |||
|
|
|||
|
|
|||
| static ConcurrentMap<IndexShardReference, String> openShardReferences; | |||
There was a problem hiding this comment.
This feels way more natural on the IndexShard level - I felt that way when I made it but couldn't find a way (at the time) to push it to IndexShard while getting the debugging information to be used as the string value here. We can pass it to IndexShard.acquirePrimaryOperationLock and acquireReplicaOperationLock but that will force a toString on the request object. How about making the reason an Object type and only call toString on it in IndexShard?
There was a problem hiding this comment.
or even better a reason supplier called on demand?
There was a problem hiding this comment.
I agree that it's more natural there, but I prefer to keep it where it is until there's a clear need for it at the lower level.
There was a problem hiding this comment.
guys we can't have static maps on classes like this. If we want some kind of static assertions we should inject some mock services just as we do for for SearchContext in MockSearchService. I am a bit confused why we have a static factory method on literally the only impl of an interface in IndexShardReferenceImpl this smells like a design flaw to me. I think with this change together we should rather expose a pluggalbe service or move the creation into IndexService and let folks plug in a factory into IndexModule in order to impl this assertion? but static per JVM concurrent maps seems like a playing russian roulette
|
LGTM. Thanks again @jasontedor |
* master: (156 commits) Make JNA calls optional Added RPM metadata Remove PROTOTYPE from MLT.Item Remove PROTOTYPE from VersionType Fix mistake in TopHits change Remove PROTOTYPEs from highlighting Clean up some log messages Command line arguments with comma must be quoted on windows Cluster Health should run on applied states, even if waitFor=0 #17440 ingest: make concrete processor impl final, like all other processor concrete impls. Improve some test method comments. Document task id's as string in the rest spec Replace FieldStatsProvider with a method on MappedFieldType. #17334 cleanup test Remove MathUtils. #17454 Addressing review comments fix javadocs Make TranslogConfig immutable and pass TranslogGeneration as a ctor arg to Translog [reindex] Don't get rejected Remove redundant commit - #openTranslog() already commits in that case ...
This commit adds a guard preventing old cluster states from entering into the pending queue when we are following a master, and cleans old cluster states from the pending queue when processing a commit.
| @@ -164,12 +164,17 @@ public synchronized void markAsProcessed(ClusterState state) { | |||
| currentMaster | |||
| ); | |||
| } | |||
| } else if (state.supersedes(pendingState) && pendingContext.committed()) { | |||
| } else if (state.version() >= pendingState.version()) { | |||
| assert state.supersedes(pendingState) || ( | |||
There was a problem hiding this comment.
is we're here, then the masters are equal. In this case supersedes == false, means version equality (because it's can't be lower. Also state.nodes().getMasterNodeId() is never null (and assigned to currentMaster.getId())) . I'm not sure what we're asserting than. Also note that it disables the next if clause, checking for uuid equality, so maybe we want to do the next if clause first?
There was a problem hiding this comment.
Removed assertion, reordered checks in c2ed5a1.
|
@bleskes I've addressed all comments from the latest feedback round. |
This commit removes a superfluous check when validing incoming cluster states. The check in question prevents out-of-order cluster states from the same master from entering the queue. However, such out-of-order cluster states will be cleaned from the queue when a commit message for that cluster state arrives or a commit message for any higher-versioned cluster state arrives.
|
LGTM again. Thanks for all the hard work @jasontedor |
clintongormley
added
:Distributed/Distributed
Recent work on the distributed systems front brings us to a point where
we are very close to being able to enable the acked indexing test. We
have chased down one additional failure that will require the primary
terms work to address, but let's get this preparatory work in.
Closes #7572