Conversation
| int rc = REDISMODULE_OK; | ||
| if (depleteInBackground) { | ||
| sync_ref = DepleterSync_New(req->nrequests, params->synchronize_read_locks); | ||
| sync_ref = DepleterSync_New(req->nrequests, true); |
There was a problem hiding this comment.
So now the decision wether to take the read lock is dependent on which function we use to build the pipeline?
HybridRequest_BuildDepletionPipeline \ HybridRequest_BuildDepletionPipeline ?
Honestly I dont like it, I prefer the parameter approach
Anyway, its not crucial and leaving it to you to decide
There was a problem hiding this comment.
when we operate at the shard level we need to take the index lock
when we operate at the coord level we don't
we have the internal boolean in the parse code but it doesn't tell us if we are in coordinator or shard.
because in standalone we also need to take the lock and internal would be false in such a case.
for now will leave as is
Itzikvaknin
left a comment
Itzikvaknin
left a comment
There was a problem hiding this comment.
Only typos, spellcheck was lazy this time
code review comment Co-authored-by: Itzikvaknin <[email protected]>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7404 +/- ##
==========================================
- Coverage 85.17% 85.11% -0.06%
==========================================
Files 346 346
Lines 53049 53048 -1
Branches 13773 13773
==========================================
- Hits 45184 45154 -30
- Misses 7671 7700 +29
Partials 194 194
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
kei-nan
deleted the
master_jk_hybrid_avoid_taking_read_lock_in_coordindator
branch
… Lock (#7404) * always lock in shard sepletion flow, never lock in coordinator depletion flow * Apply suggestions from code review code review comment Co-authored-by: Itzikvaknin <[email protected]> --------- Co-authored-by: Itzikvaknin <[email protected]> (cherry picked from commit 9589f1d)
|
Successfully created backport PR for |
…x Read Lock (#7430) MOD-12489: FT.HYBRID - Coordinator Deadlock - Avoid Taking Index Read Lock (#7404) * always lock in shard sepletion flow, never lock in coordinator depletion flow * Apply suggestions from code review code review comment --------- (cherry picked from commit 9589f1d) Co-authored-by: kei-nan <[email protected]> Co-authored-by: Itzikvaknin <[email protected]>
3 participants
A deadlock was found since the read locks for the index were held by the depleters.
Describe the changes in the pull request
A clear and concise description of what the PR is solving, including:
Main objects this PR modified
Mark if applicable
Note
Stop taking index read locks in distributed hybrid depleters to prevent deadlocks; remove the synchronize_read_locks parameter and set locking explicitly where needed.
src/coord/hybrid/dist_hybrid_plan.cpp, create depleter sync withfalseto avoid index read locks during distributed depletion.src/hybrid/hybrid_request.c, whendepleteInBackgroundis true, create depleter sync withtrue(explicitly enabling coordination), decoupled from params.HybridPipelineParams.synchronize_read_locksfromsrc/pipeline/pipeline.hand its usage.hybridParams->synchronize_read_locksinsrc/hybrid/parse_hybrid.c.Written by Cursor Bugbot for commit fe37ea5. This will update automatically on new commits. Configure here.