Repository navigation
Remove explainUnassignedShardAllocation side effects - #157766
Conversation
Currently, explainUnassignedShardAllocation tries to query the current state of the fetches. These calls come from threads other than the master-service thread. However, when calling fetchData, it potentially triggers additional fetches. This causes the race condition in the failing test ClusterDisruptionIT.testAckedIndexing where explainUnassignedShardAllocation gets an open fetch from the map which has become closed by the time we call fetchData. This change removes the ability for the explainUnassignedShardAllocation flow to mutate allocator fetch state Closes: #155449
|
Hi @joshua-adams-1, I've created a changelog YAML for you. |
🔍 Preview links for changed docs⏳ Building and deploying preview... View progress This comment will be updated with preview links when the build is complete. |
|
NB the test failure was on 9.3 (no longer supported), so I don't need to remove a muted-test entry, and I will backport this fix to 9.5 and 9.4 only. I don't need to backport to 8.19 since the debug logging was introduced in 9.2 in #133958 NB |
ℹ️ Important: Docs version tagging👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version. We use applies_to tags to mark version-specific features and changes. Expand for a quick overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
| ShardRouting unassignedShard, | ||
| RoutingAllocation allocation, | ||
| Logger logger, | ||
| boolean allocate |
There was a problem hiding this comment.
The hierarchy of classes in this part of the codebase is a little confusing. Since this is called by both the allocation and explain flows, I needed a way to distinguish between them, and a simple boolean flag seemed to be the best way (I couldn't find some other way to just "know" which path we're on without being explicit with a flag).
|
Pinging @elastic/es-distributed (Team:Distributed) |
|
@DaveCTurner Hey, a gentle nudge if you have time this week for a review |
|
I don't particularly like adding a new Yet, is this really doing anything different from Could we also use |
…elasticsearch into tf-gateway-allocator-cluster-it
| } | ||
|
|
||
| // Allocation should run only off the master thread | ||
| assert MasterService.assertMasterUpdateOrTestThread(); |
There was a problem hiding this comment.
There's always the possibility this causes a test failure down the line, but that would be proof of a bug, right? So it's okay to add this assertion?
There was a problem hiding this comment.
Yep exactly. Tho the comment says "off the master thread" and I think you mean "on the master thread".
There was a problem hiding this comment.
Ahaha the irony is that I wrote this comment and not an LLM 🤦
DaveCTurner
left a comment
There was a problem hiding this comment.
LGTM, with a couple of optional nits
| final FetchResult<NodeGatewayStartedShards> shardState = fetchData(unassignedShard, allocation); | ||
| if (shardState.hasData() == false) { | ||
| allocation.setHasPendingAsyncFetch(); | ||
| if (explain == false) { |
There was a problem hiding this comment.
I don't think we need this extra if here, it doesn't matter if we set this flag while explain is true does it?
| if (shardStores.hasData() == false) { | ||
| logger.trace("{}: ignoring allocation, still fetching shard stores", unassignedShard); | ||
| allocation.setHasPendingAsyncFetch(); | ||
| if (explain == false) { |
| * <p> | ||
| * This method may run on threads other than the master service thread. Implementations must not start new | ||
| * fetches or otherwise mutate allocator fetch state. |
There was a problem hiding this comment.
Not sure this comment is all that helpful (unlike the assertions that check we're on the expected thread)
Currently, `explainUnassignedShardAllocation` tries to query the current state of the fetches. These calls come from threads other than the master-service thread. However, when calling `fetchData`, it potentially triggers additional fetches. This causes the race condition in the failing test `ClusterDisruptionIT.testAckedIndexing` where `explainUnassignedShardAllocation` gets an open fetch from the map which has become closed by the time we call `fetchData`. This change removes the ability for the `explainUnassignedShardAllocation` flow to mutate allocator fetch state Closes: #155449
Currently, `explainUnassignedShardAllocation` tries to query the current state of the fetches. These calls come from threads other than the master-service thread. However, when calling `fetchData`, it potentially triggers additional fetches. This causes the race condition in the failing test `ClusterDisruptionIT.testAckedIndexing` where `explainUnassignedShardAllocation` gets an open fetch from the map which has become closed by the time we call `fetchData`. This change removes the ability for the `explainUnassignedShardAllocation` flow to mutate allocator fetch state Closes: #155449
Currently,
explainUnassignedShardAllocationtries to query the current state of the fetches. These calls come from threads other than the master-service thread. However, when callingfetchData, it potentially triggers additional fetches. This causes the race condition in the failing testClusterDisruptionIT.testAckedIndexingwhereexplainUnassignedShardAllocationgets an open fetch from the map which has become closed by the time we callfetchData. This change removes the ability for theexplainUnassignedShardAllocationflow to mutate allocator fetch stateCloses: #155449