fix(replication): Fix journal record flush #3075
Conversation
dranikpg
changed the title
Signed-off-by: Vladislav Oleshko <[email protected]>
| @@ -34,6 +34,7 @@ class Journal { | |||
|
|
|||
| LSN GetLsn() const; | |||
|
|
|||
| // WHY NO COMMENTS?? :( | |||
There was a problem hiding this comment.
specifically why this flag was introduced, and whether it's safe to suspend in the general case from custom journal handlers
| @@ -18,8 +18,7 @@ using Payload = journal::Entry::Payload; | |||
| void RecordJournal(const OpArgs& op_args, string_view cmd, ArgSlice args, uint32_t shard_cnt, | |||
| bool multi_commands) { | |||
| VLOG(2) << "Logging command " << cmd << " from txn " << op_args.tx->txid(); | |||
| op_args.tx->LogJournalOnShard(op_args.shard, Payload(cmd, args), shard_cnt, multi_commands, | |||
| false); | |||
| op_args.tx->LogJournalOnShard(op_args.shard, Payload(cmd, args), shard_cnt, multi_commands, true); | |||
There was a problem hiding this comment.
The call to RecordJournal is always from non auto journal commands where we call RecordJournal inside the command callback. Untill now we did not allow preemption in callbacks, we have some assumptions in code that callbacks do not preempt. We do need to fix them as part of the effort of Big values serialization Shahar is working on
There was a problem hiding this comment.
I agree with @dranikpg that a comment is missing that explains why we need to avoid preemption.
Also, I am SURE it is possible to fix the logic so that we won't aggregate gigabytes of memory even if we do not allow preemption in the callback.
There was a problem hiding this comment.
Yes I agree. I remember that this fix - https://github.com/dragonflydb/dragonfly/pull/3016/files should fix the problem. I will check the issue again
|
Closed for #3084 |
Fixes a bug with buffer not being flushed