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.

Sign up for GitHub

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

Jump to bottom

fix: forbid parallel save operations #2172

Merged

BorysTheDev merged 18 commits into main from parallelSaveOperationsFix

Nov 21, 2023

Contributor

BorysTheDev commented Nov 14, 2023

BorysTheDev requested review from chakaz, adiholden and dranikpg

November 14, 2023 09:31

dranikpg reviewed

View reviewed changes

Contributor

dranikpg left a comment •

edited

I like the approach. Please double check which state switches rely on the final state (regardless of switch) and which rely solely on the switch (I assume it's even only the one in the issue)... If the regression tests pass it should be close to correct 😆

src/server/server_family.cc Outdated Show resolved Hide resolved

src/server/replica.cc Outdated Show resolved Hide resolved

src/server/detail/save_stages_controller.cc Outdated Show resolved Hide resolved

romange reviewed

View reviewed changes

src/server/replica.cc Show resolved Hide resolved

Collaborator

adiholden commented Nov 14, 2023

please add a regression test that issues 2 save commands at the same time and make sure one of them is rejected

adiholden reviewed

View reviewed changes

tests/dragonfly/snapshot_test.py Outdated Show resolved Hide resolved

adiholden reviewed

View reviewed changes

tests/dragonfly/snapshot_test.py Outdated Show resolved Hide resolved

adiholden reviewed

View reviewed changes

src/server/dflycmd.cc Outdated Show resolved Hide resolved

BorysTheDev requested a review from dranikpg

November 15, 2023 16:21

dranikpg previously approved these changes

View reviewed changes

tests/dragonfly/replication_test.py Outdated Show resolved Hide resolved

adiholden reviewed

View reviewed changes

tests/dragonfly/snapshot_test.py Outdated Show resolved Hide resolved

adiholden reviewed

View reviewed changes

tests/dragonfly/replication_test.py Show resolved Hide resolved

adiholden reviewed

View reviewed changes

src/server/detail/save_stages_controller.cc Outdated Show resolved Hide resolved

BorysTheDev dismissed dranikpg’s stale review via

b6d23b2

November 16, 2023 17:22

BorysTheDev force-pushed the parallelSaveOperationsFix branch from bc5065f to f6874be Compare

November 16, 2023 17:51

BorysTheDev requested review from dranikpg and adiholden

November 16, 2023 18:51

adiholden reviewed

View reviewed changes

tests/dragonfly/snapshot_test.py Outdated Show resolved Hide resolved

adiholden reviewed

View reviewed changes

src/facade/cmd_arg_parser.cc Show resolved Hide resolved

adiholden reviewed

View reviewed changes

src/server/server_family.cc Outdated Show resolved Hide resolved

dranikpg reviewed

View reviewed changes

src/facade/cmd_arg_parser.h Show resolved Hide resolved

BorysTheDev requested a review from adiholden

November 20, 2023 09:34

adiholden reviewed

View reviewed changes

src/server/main_service.cc Show resolved Hide resolved

adiholden previously approved these changes

View reviewed changes

BorysTheDev dismissed adiholden’s stale review via

5b976d7

November 20, 2023 14:59

adiholden reviewed

View reviewed changes

src/server/dflycmd.cc Outdated Show resolved Hide resolved

adiholden reviewed

View reviewed changes

src/server/dflycmd.cc Outdated Show resolved Hide resolved

adiholden reviewed

View reviewed changes

src/server/dflycmd.cc Outdated Show resolved Hide resolved


          fix: forbid parallel save operations

73e4c1a

BorysTheDev added 15 commits

November 21, 2023 12:00


          fix: fix tests

f0b3e89


          fix: fix tests and add double save test

8206e65


          fix: remove save after TAKEOVER and fix test

fdcc008


          fix: update test_take_over_seeder according to the new shutdown behavior

e2c7a1d


          feat: add SAVE option to takeove command

9df743e


          fix: fix build, remove deprecated method

93ecb22


          fix: fix build

061b5bf


          fix: fix test

2014fbc


          fix: fix double save during shutdown

b8253a7


          fix: move switch state out of save_stages_controller

77d80d7


          fix: fix transaction_guard scope for takeover

c2653e1


          fix: change transactionGuard to ExpirationGuard

3b9368d


          fix: disable expiration for all shards

7cee79d


          test: update target_ops

0c87f49


          fix: try to tune target_ops more

3d05906

BorysTheDev force-pushed the parallelSaveOperationsFix branch from 3cb6ade to 3d05906 Compare

November 21, 2023 10:08

adiholden reviewed

View reviewed changes

src/server/dflycmd.cc Outdated Show resolved Hide resolved

adiholden reviewed

View reviewed changes

tests/dragonfly/replication_test.py Outdated

                           # Df is blazingly fast, so by the time we tick a second time on
                           # line 1674, DF already replicated all the data so the assertion
                           # at the end of the test will always fail
-                          fill_task = asyncio.create_task(seeder.run())
+                          fill_task = asyncio.create_task(seeder.run(target_ops=40000000))

Collaborator

adiholden Nov 21, 2023

why did you add target_ops?
The flow of the test should be seeder run until we call below seeder.stop()

What was failing that caused you to change this?

Contributor Author

BorysTheDev Nov 21, 2023

the test failed on the build machine, but locally everything was correct. So according to comments I've increased target_ops

Collaborator

adiholden Nov 21, 2023

This comment was written in this PR https://github.com/dragonflydb/dragonfly/pull/1970/files and I believe its not relevant now as we dont use target ops. We need to find why this failed but for now please remove this change.


          refactor: remove expirationGuard

394a6c4

BorysTheDev requested a review from adiholden

November 21, 2023 10:55


          test: remove extra argument

e56afd2

adiholden approved these changes

View reviewed changes

BorysTheDev merged commit e6f3522 into main

10 checks passed

BorysTheDev deleted the parallelSaveOperationsFix branch

November 21, 2023 11:56

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment