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.

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

add Transactions and Checkpoint to C API #2236

Closed
wants to merge 5 commits into from
Closed

add Transactions and Checkpoint to C API #2236

wants to merge 5 commits into from

Conversation

boolean5
Copy link
Contributor

@boolean5 boolean5 commented May 1, 2017

I've added functions to the C API to support Transactions as requested in #1637 and to support Checkpoint.

I have also added the corresponding tests to c_test.c

For now, the following is omitted:

  1. Optimistic Transactions
  2. The column family variation of functions

@facebook-github-bot
Copy link
Contributor

@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@siying
Copy link
Contributor

siying commented May 2, 2017

Thank you for contributing. Can you follow the formatting convention in this code base (like 2-char indenting)?

@facebook-github-bot
Copy link
Contributor

@boolean5 updated the pull request - view changes - changes since last import

@boolean5
Copy link
Contributor Author

boolean5 commented May 3, 2017 •
edited
Loading

Done. I believe it is ok now.
Also, I can see that the Travis CI build failure is irrelevant to my code, as it was already happening in commit a2b0521 , on which I based my work.

Please, let me know if there is anything else I can do to help merge this pull request.

@JelteF
Copy link
Contributor

JelteF commented May 11, 2017

This looks great! I was just looking for checkpoint support in the C API. Anything holding op the merge?

@JelteF JelteF mentioned this pull request May 11, 2017
Closed
@facebook-github-bot
Copy link
Contributor

@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

yiwu-arbug
yiwu-arbug suggested changes May 15, 2017
View reviewed changes
Copy link
Contributor

@yiwu-arbug yiwu-arbug left a comment

Choose a reason for hiding this comment

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

See comment regarding deleting the transaction.

Also please run make format once to fix formatting.

db/c_test.c
CheckTxnDBGet(txn_db, roptions, "foo", NULL);

// begin a transaction
txn = rocksdb_transaction_begin(txn_db, woptions, txn_options, NULL);
Copy link
Contributor

Choose a reason for hiding this comment

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

ASAN complains about txn is not deleted. How about adding a rocksdb_transaction_destroy() method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I added the method.
Let me know if I should fix anything else.

@facebook-github-bot
Copy link
Contributor

@boolean5 updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@yiwu-arbug
Copy link
Contributor

@boolean5 Unfortunately there's merge conflict now. Do you mind rebase? Thanks!

@boolean5
add Transactions and Checkpoint to C API
ed041da 
remove DestroyDb for TransactionDB

add Options for Transaction and TransactionDB

add transactions to C API - batch 2

add Checkpoint

last fixes
@boolean5
follow formatting convention
9d39139
@boolean5
add rocksdb_transaction_destroy() and fix formatting
d5d7056
@facebook-github-bot
Copy link
Contributor

@boolean5 updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

yiwu-arbug
yiwu-arbug suggested changes May 16, 2017
View reviewed changes
db/c_test.c
CheckTxnDBGet(txn_db, roptions, "foo", "hello");

// reuse old transaction
txn = rocksdb_transaction_begin(txn_db, woptions, txn_options, txn);
Copy link
Contributor

Choose a reason for hiding this comment

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

you need to destroy the old txn before creating a new one.

Copy link
Contributor Author

@boolean5 boolean5 May 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I thought the whole point of the last parameter of rocksdb_transaction_begin was to avoid deallocating and reallocating a transaction, as stated here: https://github.com/facebook/rocksdb/blob/master/include/rocksdb/utilities/transaction_db.h#L158
Isn't it so?

Copy link
Contributor

Choose a reason for hiding this comment

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

@boolean5 sorry, I misread it. But ASAN is still complaining about the txn not getting destroy. Mind debug it? You can run COMPILE_WITH_ASAN=1 make c_test && ./c_test to reproduce.

Copy link
Contributor Author

@boolean5 boolean5 May 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Found it. The bug was in c.cc, in rocksdb_transaction_begin(), where I was allocating a new transaction even when an old one was passed as an argument. Thanks for pointing this out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome!

@facebook-github-bot
Copy link
Contributor

@boolean5 updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@boolean5 updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@tamird
Copy link
Contributor

tamird commented May 17, 2017

Looks like this broke the appveyor build https://ci.appveyor.com/project/Facebook/rocksdb/build/1.0.4437

@boolean5
Copy link
Contributor Author

Seems that the appveyor build has been broken with the same compilation messages at least since commit f720796 : https://ci.appveyor.com/project/Facebook/rocksdb/build/1.0.4421

@tamird
Copy link
Contributor

tamird commented May 17, 2017 via email

@yiwu-arbug
Copy link
Contributor

My bad. @sagar0 is fixing it with #2309.

@sagar0
Copy link
Contributor

sagar0 commented May 17, 2017

Just merged #2309; it will land shortly.
@boolean5 can you please rebase this PR once #2309 lands, so that a new appveyor builds gets kicked off?

@yiwu-arbug
Copy link
Contributor

Thanks @sagar0! This PR has been merged, so nothing needs to be done.

@sagar0
Copy link
Contributor

sagar0 commented May 17, 2017

Oh, my bad. Didn't notice that this PR has already been merged.

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

@yiwu-arbug yiwu-arbug yiwu-arbug requested changes

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@boolean5 @facebook-github-bot @siying @JelteF @yiwu-arbug @tamird @sagar0