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

User Defined Timestamp APIs #77

Merged
merged 10 commits into from
Aug 8, 2022

Conversation

muthukrishnan24
Copy link
Contributor

@muthukrishnan24 muthukrishnan24 commented Jul 24, 2022

Adds bindings for User Defined Timestamp APIs

@muthukrishnan24
Copy link
Contributor Author

@linxGnu

@linxGnu
Copy link
Owner

linxGnu commented Jul 28, 2022

I am sorry for late reply due to my job. I will take a look and feedback soon.

thank you for the contribution

db.go Outdated Show resolved Hide resolved
db.go Outdated Show resolved Hide resolved
iterator.go Outdated Show resolved Hide resolved
options_compaction.go Outdated Show resolved Hide resolved
transaction.go Show resolved Hide resolved
transaction.go Show resolved Hide resolved
@muthukrishnan24 muthukrishnan24 requested a review from linxGnu August 2, 2022 09:26
@muthukrishnan24
Copy link
Contributor Author

Made the requested changes. Please check @linxGnu

db.go Outdated Show resolved Hide resolved
db.go Outdated Show resolved Hide resolved
@linxGnu
Copy link
Owner

linxGnu commented Aug 4, 2022

@muthukrishnan24

Almost LGTM, just 2 request changes above.

And could you please add more tests. It's important to ensure ops with timestamp works. AFAIK, timestamp feature is currently under experiment?

@muthukrishnan24
Copy link
Contributor Author

AFAIK, timestamp feature is currently under experiment?

taken from here

It's still experimental in the sense that:

   - it's currently supported in a subset of RocksDB features: Get, Put, WriteBatch (with only Put and Delete), Delete, Iterator, basic Garbage collection not necessarily the most efficient implementation), and
   - new APIs and/or options may be added, and
   - (less likely) existing user-defined timestamp APIs and/or options are also subject to change in future RocksDB releases

@linxGnu
Copy link
Owner

linxGnu commented Aug 4, 2022

@muthukrishnan24 thank you for the info.

could you please fix my comments above and especially: adding tests

@muthukrishnan24
Copy link
Contributor Author

muthukrishnan24 commented Aug 7, 2022

Fixed your comments, Added test cases. Please check @linxGnu

@muthukrishnan24 muthukrishnan24 requested a review from linxGnu August 7, 2022 11:17
@linxGnu
Copy link
Owner

linxGnu commented Aug 8, 2022

Its absolutely great job @muthukrishnan24
I will merge your PR and prepare for next release.

@linxGnu linxGnu merged commit aac6cb3 into linxGnu:master Aug 8, 2022
@muthukrishnan24 muthukrishnan24 deleted the adapt_rocksdb_ts branch August 13, 2022 14:25
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.

2 participants