-
Notifications
You must be signed in to change notification settings - Fork 1k
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
fix: lsn check failure #4881
fix: lsn check failure #4881
Conversation
Signed-off-by: kostas <kostas@dragonflydb.io>
@@ -168,8 +168,7 @@ bool MultiCommandSquasher::ExecuteStandalone(facade::RedisReplyBuilder* rb, Stor | |||
return true; | |||
} | |||
|
|||
OpStatus MultiCommandSquasher::SquashedHopCb(Transaction* parent_tx, EngineShard* es, | |||
RespVersion resp_v) { | |||
OpStatus MultiCommandSquasher::SquashedHopCb(EngineShard* es, RespVersion resp_v) { |
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.
parent_tx
was not used.
@@ -258,11 +256,6 @@ bool MultiCommandSquasher::ExecuteSquashed(facade::RedisReplyBuilder* rb) { | |||
shard_set->AddL2(i, cb); | |||
} | |||
bc->Wait(); | |||
#else |
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.
removed dead code
@@ -1129,7 +1129,7 @@ bool Transaction::ScheduleInShard(EngineShard* shard, bool execute_optimistic) { | |||
DVLOG(3) << "Lock granted " << lock_granted << " for trans " << DebugId(); | |||
|
|||
// Check if we can run immediately | |||
if (shard_unlocked && execute_optimistic && lock_granted) { | |||
if (lock_granted && execute_optimistic) { |
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.
lock_granted = shard_unlocked && keys_unlocked;
so checking explicitly for shard_unlocked
is redundant
// Return true if no replicas are registered or if all replicas reached stable sync | ||
// Used in debug populate to DCHECK insocsistent flows that violate transaction gurantees | ||
bool AreAllReplicasInStableSync() const { | ||
auto roles = dfly_cmd_->GetReplicasRoleInfo(); |
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 think this function takes several mutexes and better we use it not so frequently therefor I suggest to do the check in debug populate only when we start the debug populate and we can also do when we finish.
Not for every command invocation
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.
No objection, my rationale was that we don't care since it's not used in production. Happy to change that though.
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.
you are right it does not affect prod but it might slow the debug populate in our tests
Test
test_bug_in_json_memory_tracking
failed because it usedStaticSeeder
which usesdebug populate
which is non transactional.fixes #4048
Also
test_disconnect_replica
was failing months ago but I run this for more than 5 hours without any success.