Skip to content
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

introduce --replicaof flag #1583

Merged
merged 22 commits into from
Aug 9, 2023
Merged

introduce --replicaof flag #1583

merged 22 commits into from
Aug 9, 2023

Conversation

talbii
Copy link
Contributor

@talbii talbii commented Jul 23, 2023

Closes #1381.

talbii and others added 3 commits July 23, 2023 22:22
Signed-off-by: talbii <ido@dragonflydb.io>
Signed-off-by: talbii <41526934+talbii@users.noreply.github.com>
Signed-off-by: talbii <ido@dragonflydb.io>
@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
Collaborator

@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

src/facade/reply_builder.h Outdated Show resolved Hide resolved
src/server/main_service.cc Outdated Show resolved Hide resolved
src/server/main_service.cc Outdated Show resolved Hide resolved
src/server/main_service.cc Outdated Show resolved Hide resolved
src/server/main_service.cc Outdated Show resolved Hide resolved
src/server/server_family.cc Outdated Show resolved Hide resolved
src/server/server_family.cc Outdated Show resolved Hide resolved
src/server/server_family.h Outdated Show resolved Hide resolved
src/server/server_family.cc Outdated Show resolved Hide resolved
src/server/server_family.cc Outdated Show resolved Hide resolved
Signed-off-by: talbii <ido@dragonflydb.io>
Signed-off-by: talbii <ido@dragonflydb.io>
…roduce `ReplicaOfFlag`

Signed-off-by: talbii <ido@dragonflydb.io>
@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 :)

src/server/server_family.cc Outdated Show resolved Hide resolved
src/server/server_family.cc Show resolved Hide resolved
src/server/server_family.cc Outdated Show resolved Hide resolved
src/server/server_family.cc Show resolved Hide resolved
src/server/server_family.cc Outdated Show resolved Hide resolved
src/server/server_family.cc Show resolved Hide resolved
src/server/server_family.cc Outdated Show resolved Hide resolved
src/server/server_family.cc Outdated Show resolved Hide resolved
src/server/server_family.cc Show resolved Hide resolved
src/server/server_family.cc Outdated Show resolved Hide resolved
src/server/server_family.cc Show resolved Hide resolved
src/server/server_family.cc Outdated Show resolved Hide resolved
src/server/server_family.cc Outdated Show resolved Hide resolved
src/server/server_family.cc Outdated Show resolved Hide resolved
src/server/server_family.cc Show resolved Hide resolved
src/server/server_family.cc Outdated Show resolved Hide resolved
src/server/server_family.cc Show resolved Hide resolved
src/server/server_family.cc Outdated Show resolved Hide resolved
src/server/server_family.cc Outdated Show resolved Hide resolved
src/server/server_family.cc Outdated Show resolved Hide resolved
Signed-off-by: talbii <ido@dragonflydb.io>
…until `REPLICAOF NO ONE` is recieved

Signed-off-by: talbii <ido@dragonflydb.io>
src/server/replica.cc Outdated Show resolved Hide resolved
src/server/server_family.cc Outdated Show resolved Hide resolved
src/server/server_family.h Outdated Show resolved Hide resolved
src/server/server_family.cc Outdated Show resolved Hide resolved
Signed-off-by: talbii <ido@dragonflydb.io>
Signed-off-by: talbii <ido@dragonflydb.io>
Signed-off-by: talbii <ido@dragonflydb.io>
Signed-off-by: talbii <ido@dragonflydb.io>
Signed-off-by: talbii <ido@dragonflydb.io>
src/facade/reply_builder.h Outdated Show resolved Hide resolved
src/facade/reply_builder.h Outdated Show resolved Hide resolved
src/server/replica.cc Outdated Show resolved Hide resolved
src/server/server_family.cc Outdated Show resolved Hide resolved
src/server/server_family.cc Outdated Show resolved Hide resolved
src/server/server_family.cc Outdated Show resolved Hide resolved
src/server/server_family.cc Outdated Show resolved Hide resolved
src/server/server_family.h Outdated Show resolved Hide resolved
src/server/replica.cc Outdated Show resolved Hide resolved
src/server/server_family.cc Outdated Show resolved Hide resolved
talbii and others added 4 commits August 7, 2023 09:23
Signed-off-by: talbii <ido@dragonflydb.io>
Signed-off-by: talbii <ido@dragonflydb.io>
Signed-off-by: talbii <41526934+talbii@users.noreply.github.com>
Signed-off-by: talbii <ido@dragonflydb.io>
@talbii talbii requested a review from chakaz August 7, 2023 11:38
src/facade/reply_builder.cc Outdated Show resolved Hide resolved
src/server/server_family.cc Outdated Show resolved Hide resolved
src/server/server_family.cc Outdated Show resolved Hide resolved
};
transaction->Execute(std::move(cb), true);
if (should_flush_db == ShouldFlushDb::kFlush)
FlushEntireDb(cntx->transaction);
Copy link
Collaborator

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
Collaborator

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

src/server/server_family.cc Outdated Show resolved Hide resolved
Signed-off-by: talbii <ido@dragonflydb.io>
Signed-off-by: talbii <ido@dragonflydb.io>
@talbii talbii requested a review from chakaz August 8, 2023 09:04
chakaz
chakaz previously approved these changes Aug 8, 2023
src/server/server_family.cc Outdated Show resolved Hide resolved
Signed-off-by: talbii <ido@dragonflydb.io>
tests/dragonfly/replication_test.py Outdated Show resolved Hide resolved
tests/dragonfly/replication_test.py Outdated Show resolved Hide resolved
Signed-off-by: talbii <ido@dragonflydb.io>
@talbii talbii requested a review from chakaz August 9, 2023 08:47
src/server/replica.cc Outdated Show resolved Hide resolved
src/server/replica.cc Outdated Show resolved Hide resolved
src/server/replica.cc Outdated Show resolved Hide resolved
* 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
Collaborator

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 <ido@dragonflydb.io>
@talbii talbii requested a review from chakaz August 9, 2023 10:00
Copy link
Collaborator

@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
10 checks passed
@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