Skip to content

Conversation

talbii
Copy link
Contributor

@talbii talbii commented Jul 23, 2023

Closes #1381.

@talbii talbii marked this pull request as ready for review July 23, 2023 19:28
@talbii talbii marked this pull request as draft July 23, 2023 20:11
Copy link
Contributor

@chakaz chakaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @talbii, see some comments but please feel free to reach out if I missed something or if anything's unclear

@talbii talbii marked this pull request as ready for review July 24, 2023 12:23
Copy link
Contributor

@kostasrim kostasrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work, some minor comments.

Moreover, add a test with the new flag on replication_test.py.

If you have any questions or if anything I commented is unclear, plz reach me out internally and we can jump on a call. I am here if you need me anything :)

talbii added 2 commits July 25, 2023 14:56
Signed-off-by: talbii <[email protected]>
…until `REPLICAOF NO ONE` is recieved

Signed-off-by: talbii <[email protected]>
@talbii talbii requested a review from chakaz August 7, 2023 11:38
};
transaction->Execute(std::move(cb), true);
if (should_flush_db == ShouldFlushDb::kFlush)
FlushEntireDb(cntx->transaction);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this be a problem, in that after this function finishes the transaction will be concluded afterwards? Or is it ok to Schedule() an already concluded transaction?

Copy link
Contributor Author

@talbii talbii Aug 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that I am not too sure about, but I'm not sure I understand your concern? The logic of FlushEntireDb (or FLUSHDB or Drakarys..) schedules the transaction first and then executes it, which is fine.

Are you worried that replication continues past the already-executed transaction?

Maybe @romange could chime in, I saw that he originally wrote this code 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried that transactions can't be used more than once, in which case maybe something unintended can happen after we flush the DB, when we try to use the tx again

@talbii talbii requested a review from chakaz August 8, 2023 09:04
chakaz
chakaz previously approved these changes Aug 8, 2023
@talbii talbii requested a review from chakaz August 8, 2023 09:18
@talbii talbii enabled auto-merge (squash) August 8, 2023 09:23
@talbii talbii requested a review from chakaz August 9, 2023 08:47
* an instance with '--replicaof' and then \b immediately
* sending a command.
*
* In that instance, we have to run f() on the current fiber.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what's the downside of running in current fiber? Why not always do it as such? And why not run it just via f() instead of awaiting for our own thread?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that I am not entirely sure about. Presumably this was done in order to not block the current fiber, the same fiber that runs MainReplicationFb.

About why I run f() using AwaitBrief, I was not sure that was the same outcome. I'll change it, thanks!

Signed-off-by: talbii <[email protected]>
@talbii talbii requested a review from chakaz August 9, 2023 10:00
Copy link
Contributor

@chakaz chakaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥳
Please run the tests multiple times before merging this in, to make sure they are robust

@talbii talbii merged commit 16c2353 into main Aug 9, 2023
@talbii talbii deleted the flag-replicaof branch August 9, 2023 11:42
talbii added a commit that referenced this pull request Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add replicaof command flag
4 participants