WritePrepared: commit only from the 2nd queue #5014
Closed
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
There was a problem hiding this comment.
@maysamyabandeh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@maysamyabandeh has updated the pull request. Re-import the pull request |
lth
reviewed
Feb 28, 2019
| // Should the callback also publishes the commit seq number | ||
| bool publish_seq_; | ||
| // Auxiliary batch (if there is any) | ||
| SequenceNumber aux_seq_; |
There was a problem hiding this comment.
I think it'd be useful to explain the difference between aux_batch_cnt_ and data_batch_cnt_. From what I understand this only matters for two queues?
| @@ -802,6 +809,11 @@ class WritePreparedCommitEntryPreReleaseCallback : public PreReleaseCallback { | |||
| db_->AddCommitted(prep_seq_ + i, last_commit_seq); | |||
| } | |||
| } // else there was no prepare phase | |||
| if (includes_aux_batch_) { | |||
| for (size_t i = 0; i < aux_batch_cnt_; i++) { | |||
| db_->AddCommitted(aux_seq_ + i, last_commit_seq); | |||
There was a problem hiding this comment.
Is the calculation of last_commit_seq still correct in the presence of aux_seq_ (especially with data_batch_cnt_ > 1 case)? Can we add a comment explaining this?
|
@maysamyabandeh has updated the pull request. Re-import the pull request |
lth
approved these changes
Feb 28, 2019
There was a problem hiding this comment.
@maysamyabandeh is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
lth
added a commit
to lth/rocksdb
that referenced
this pull request
Jun 11, 2019
This is a port of this PR into WriteUnprepared: facebook#5014 This also reverts this test change to restore some flaky write unprepared tests: facebook#5315 Tested with: $ gtest-parallel ./transaction_test --gtest_filter=MySQLStyleTransactionTest/MySQLStyleTransactionTest.TransactionStressTest/9 --repeat=128 [128/128] MySQLStyleTransactionTest/MySQLStyleTransactionTest.TransactionStressTest/9 (18250 ms)
facebook-github-bot
pushed a commit
that referenced
this pull request
Jun 12, 2019
Summary: This is a port of this PR into WriteUnprepared: #5014 This also reverts this test change to restore some flaky write unprepared tests: #5315 Tested with: $ gtest-parallel ./transaction_test --gtest_filter=MySQLStyleTransactionTest/MySQLStyleTransactionTest.TransactionStressTest/9 --repeat=128 [128/128] MySQLStyleTransactionTest/MySQLStyleTransactionTest.TransactionStressTest/9 (18250 ms) Pull Request resolved: #5439 Differential Revision: D15761405 Pulled By: lth fbshipit-source-id: ae2581fd942d8a5b3f9278fd6bc3c1ac0b2c964c
vagogte
pushed a commit
to vagogte/rocksdb
that referenced
this pull request
Jun 18, 2019
Summary: This is a port of this PR into WriteUnprepared: facebook#5014 This also reverts this test change to restore some flaky write unprepared tests: facebook#5315 Tested with: $ gtest-parallel ./transaction_test --gtest_filter=MySQLStyleTransactionTest/MySQLStyleTransactionTest.TransactionStressTest/9 --repeat=128 [128/128] MySQLStyleTransactionTest/MySQLStyleTransactionTest.TransactionStressTest/9 (18250 ms) Pull Request resolved: facebook#5439 Differential Revision: D15761405 Pulled By: lth fbshipit-source-id: ae2581fd942d8a5b3f9278fd6bc3c1ac0b2c964c
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.
When two_write_queues is enabled we call ::AddPrepared only from the main queue, which writes to both WAL and memtable, and call ::AddCommitted from the 2nd queue, which writes only to WAL. This simplifies the logic by avoiding concurrency between AddPrepared and also between AddCommitted. The patch fixes one case that did not conform with the rule above. This would allow future refactoring. For example AdvaneMaxEvictedSeq, which is invoked by AddCommitted, can be simplified by assuming lack of concurrent calls to it.