-
Notifications
You must be signed in to change notification settings - Fork 13
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
test: align space_index_test with the transactional DDL #157
Merged
oleg-jukovec
merged 1 commit into
tarantool:master
from
mkostoevr:m.kostoev/tarantool-8751-fix
Sep 15, 2023
Merged
test: align space_index_test with the transactional DDL #157
oleg-jukovec
merged 1 commit into
tarantool:master
from
mkostoevr:m.kostoev/tarantool-8751-fix
Sep 15, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
olegrok
reviewed
Sep 15, 2023
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.
olegrok
reviewed
Sep 15, 2023
olegrok
approved these changes
Sep 15, 2023
oleg-jukovec
approved these changes
Sep 15, 2023
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.
Thank you for the patch!
If the tarantool/tarantool#8751 is merged, the space drop is transactional, so it only yields once on the space drop operation finish. This causes the test fail [1] because of unexpected state in the expirationd task. This patch fixes the problem by wrapping space drop and recreation into a transaction. More informative description of what the issue is: Consider the following facts: a. If the PR mentioned above is merged, the space drop yields on the full DDL operation end. After that point, the space is dropped entirely (no indexes or entries in the `_space` space exist). b. If the expirationd task's `atomic_iteration` option is set to false (which is the default used in the test), each tuple drop causes the expirationd task fiber yield. So here's what happens if the fact 'a' is false (the PR mentioned above is not merged): 1. The test fiber creates the expirationd task and yields into it. 2. The expirationd task drops the first tuple from the given space and yields back to the test fiber. 3. The test fiber does the `space:drop()`, which is not atomic, so 4. the drop yields on the primary index drop. Control is returned to the expirationd task. 5. The expirationd task: 1. Finishes the space iteration (there was only one tuple in the given space) 2. Uses the space (`box.space[task.space].engine == "vinyl"`) we wanted to drop in the test fiber. 3. Goes sleep, which yields to the test fiber. 6. The test fiber continues the space drop, finishes it (the next yields happening during the space drop do not return the control back to the expirationd task though, because it sleeps and it's sleep timer hasn't expired yet). 7. The test fiber recreates the space, and finally, when it's time to switch back to the expirationd task, the space exists again like no space drop happened. But if the space drop is atomic things change starting from the step 3: the space drop only yields on the whole DDL transaction commit. After that point, the space does not exist anymore. So: 4. The atomic space drop yields to the expirationd task on commit. 5. The expirationd task: 1. Finishes the iteration and checks if the space's engine is vinyl in order to update the vinyl_assumed_space_len, but the space does not exist anymore, so it fails. This causes the expirationd task restart. The last mentioned step increments the restart counter so the test check fails. In order to mitigate that the patch wraps the space drop and recreation into a transaction, so the test fiber only yields when all the required changes are applied, so the expirationd task does not see the dropped space and only works with the original or recreated space. The tranactioning is only done for Tarantool version 2.2.1 or newer because in the version the single-yield transactional DDL had been introduced. 1. https://github.com/tarantool/tarantool/actions/runs/6186475380/job/16795081554
oleg-jukovec
added a commit
that referenced
this pull request
Mar 25, 2024
Overview The release introduces a role for Tarantool 3.0. Breaking changes None. New features Tarantool 3.0 role for expirationd (#160). Testing Updated the 'space_index_test.lua' to drop and recreate the test space atomically. This prevents the space access failure in the expirationd task fiber if the `space:drop` function is transactional (#157). Updated version of `luatest` in `make deps` to 1.0.1 to support Tarantool 3.0 role tests (#160).
Merged
oleg-jukovec
added a commit
that referenced
this pull request
Mar 25, 2024
Overview The release introduces a role for Tarantool 3.0. Breaking changes None. New features Tarantool 3.0 role (#160). Testing Updated the 'space_index_test.lua' to drop and recreate the test space atomically. This prevents the space access failure in the expirationd task fiber if the `space:drop` function is transactional (#157). Updated version of `luatest` in `make deps` to 1.0.1 to support Tarantool 3.0 role tests (#160).
oleg-jukovec
added a commit
that referenced
this pull request
Mar 25, 2024
Overview The release introduces a role for Tarantool 3.0. Breaking changes None. New features Tarantool 3.0 role (#160). Testing Updated the 'space_index_test.lua' to drop and recreate the test space atomically. This prevents the space access failure in the expirationd task fiber if the `space:drop` function is transactional (#157). Updated version of `luatest` in `make deps` to 1.0.1 to support Tarantool 3.0 role tests (#160).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If the tarantool/tarantool#8751 is merged, the space drop is transactional, so it only yields once on the space drop operation finish. This causes the test fail [1] because of unexpected state in the expirationd task.
This patch fixes the problem by wrapping space drop and recreation into a transaction.
More informative description of what the issue is:
Consider the following facts:
_space
space exist).atomic_iteration
option is set to false (which is the default used in the test), each tuple drop causes the expirationd task fiber yield.So here's what happens if the fact 1 is false (the PR mentioned above is not merged):
space:drop()
, which is not atomic, sobox.space[task.space].engine == "vinyl"
) we wanted to drop in the test fiber.But if the space drop is atomic things change starting from the step 3: the space drop only yields on the whole DDL transaction commit. After that point, the space does not exist anymore. So:
The last mentioned step increments the restart counter so the test check fails.
In order to mitigate that the patch wraps the space drop and recreation into a transaction, so the test fiber only yields when all the required changes are applied, so the expirationd task does not see the dropped space and only works with the original or recreated space.
The tranactioning is only done for Tarantool version 2.2.1 or newer because in the version the single-yield transactional DDL had been introduced.