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

Use the RocksDB WriteBatchWithIndex to implement the read-your-own-writes in transaction #1287

Merged
merged 13 commits into from
Mar 4, 2023
Merged

Use the RocksDB WriteBatchWithIndex to implement the read-your-own-writes in transaction #1287

merged 13 commits into from
Mar 4, 2023

Conversation

git-hulk
Copy link
Member

@git-hulk git-hulk commented Feb 28, 2023

By default, the writes cannot be seen in transactions if the WriteBatch
wasn't committed. To mitigate this issue, RocksDB offers the WriteBatchWithIndex
to make the uncommitted writes visible if needed.

For more information can see: https://rocksdb.org/blog/2015/02/27/write-batch-with-index.html

This PR looks not easy to review at first glance, but it will be easier if review commits one by one:

  • The first commit adds the function GetWriteBatch in storage to wrap the WriteBatch for the write operations, making it possible to group and write at once in the EXEC command
  • The second commit supports using WriteBatchIndex in transaction mode so that those uncommitted writes can be seen in the same transaction
  • The third commit replaces the DB's Get and NewIterator API with WriteBatchIndex to allow visit its own writes in transaction mode.

Copy link
Member

@mapleFU mapleFU left a comment

Choose a reason for hiding this comment

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

I didn't review it carefully, so maybe I made a mistake, but my point is that this changes too much.

Assume that a transaction is execution, it doesn't block all execution threads, so, the things would be trickey:

  1. Storage (which is shared among system) would be mark as txn. My point is that if multiple thread call exec, which would win?
  2. When an non-txn get executing when txn is executing, what would it happens?

I suggest that rename it to ctx, and rather than save txn flag to storage, encapsulate it into a per-request context. And rather than transaction, this maybe better called a batch, because maybe no conflict detection here.

Updated:

Seems that exclusive flag would make exec holds and write lock. But still argue that would a exception cause the txn abort.

src/storage/storage.cc Outdated Show resolved Hide resolved
src/storage/storage.cc Show resolved Hide resolved
src/commands/cmd_txn.cc Outdated Show resolved Hide resolved
Copy link
Member

@mapleFU mapleFU left a comment

Choose a reason for hiding this comment

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

Other LGTM. Though I still think put txn_write_batch_ in storage in a bit trickey, can we add some description here, for it can only used by exclusive command?

src/storage/storage.cc Show resolved Hide resolved
src/storage/storage.cc Show resolved Hide resolved
@git-hulk
Copy link
Member Author

git-hulk commented Mar 1, 2023

Other LGTM. Though I still think put txn_write_batch_ in storage in a bit trickey, can we add some description here, for it can only used by exclusive command?

Thanks for your kind review. Agreed to add more comments to explain why the current implementation is right. I'm also thinking about how to allow multi transactions to improve performance because it's unnecessary to be exclusive command if we can promise the commit all or none. But can retrospect this after this PR and the watch/unwatch feature are ready.

@mapleFU
Copy link
Member

mapleFU commented Mar 1, 2023

Just parallel exec can be put into a "Session.Context" rather than Storage. A exec can write to it own session, and read first it own session.

Though it's ok, but it needs detail considering. For know, I think txn related is all under exclusive mode, because they only used by "Exec", which is exclusive.

@git-hulk
Copy link
Member Author

git-hulk commented Mar 1, 2023

Just parallel exec can be put into a "Session.Context" rather than Storage. A exec can write to it own session, and read first it own session.

Yes, the transaction state indeed is a connection state, but it needs some refactoring work to support binding transaction conetext in underlying storage type implementation. We won't do that until this PR and watch/unwatch are supported.

@torwig
Copy link
Contributor

torwig commented Mar 1, 2023

In general, LGTM.

mapleFU
mapleFU previously approved these changes Mar 2, 2023
Copy link
Member

@mapleFU mapleFU left a comment

Choose a reason for hiding this comment

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

Other LGTM here, but I still think we need more log and comments.

src/storage/storage.cc Outdated Show resolved Hide resolved
read-your-own-writes in transaction

By default, the writes cannot be seen its own writes if the WriteBatch
wasn't committed in transaction. To mitigate this issue, RocksDB offers
the WriteBatchWithIndex to make the uncommitted writes can be visited.

For more information can see: https://rocksdb.org/blog/2015/02/27/write-batch-with-index.html
@git-hulk git-hulk requested review from PragmaTwice, mapleFU, torwig, ShooterIT and caipengbo and removed request for mapleFU March 2, 2023 16:10
src/storage/storage.cc Show resolved Hide resolved
src/storage/storage.cc Outdated Show resolved Hide resolved
src/storage/storage.cc Show resolved Hide resolved
src/storage/storage.cc Show resolved Hide resolved
@git-hulk git-hulk requested review from torwig and removed request for mapleFU March 3, 2023 05:00
@git-hulk git-hulk requested a review from mapleFU March 3, 2023 05:01
Copy link
Member

@mapleFU mapleFU left a comment

Choose a reason for hiding this comment

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

Seems that incoming changes is that write with txn_batch_mode_ changed to writeToDb, and don't rely on txn_batch_mode_.

Currently I've no problem, but I hope we can moke error response to test this in the future. Maybe you can use sync_point in rocksdb ( I've port one of it: https://github.com/mapleFU/sync_point , but you can use it in rocksdb) to mock partial exec error in testing.

src/cluster/slot_import.cc Show resolved Hide resolved
Copy link
Contributor

@torwig torwig left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@caipengbo caipengbo left a comment

Choose a reason for hiding this comment

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

LGTM, a clear implementation.

src/storage/storage.h Show resolved Hide resolved
@git-hulk
Copy link
Member Author

git-hulk commented Mar 4, 2023

Thanks all, merging…

@git-hulk git-hulk merged commit be2a81d into apache:unstable Mar 4, 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.

Atomic commit write batches in the command EXEC [BUG] Transaction can't guarantee atomicity
5 participants