-
Notifications
You must be signed in to change notification settings - Fork 583
Implement RocksDB compatibility test in Python #17950
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
Conversation
| count = 0 | ||
| # --- Iterate over all keys --- | ||
| rocksdb.rocksdb_iter_seek_to_first(iter_) | ||
| while count < 10 and rocksdb.rocksdb_iter_valid(iter_): |
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.
why 10?
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.
It's just a random number, I can add it to params.
| print(f"Found KV-pair: {key_buf[:].hex()} -> {val_buf[:].hex()}") | ||
|
|
||
| rocksdb.rocksdb_iter_next(iter_) | ||
| count += 1 |
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.
NIT: Ugly count += 1. looks like count is not used further in the script. we can replace while with:
for _ in range(10):
if not rocksdb.rocksdb_iter_valid(iter_):
break
| rocksdb.rocksdb_iter_next(iter_) | ||
| count += 1 | ||
|
|
||
| # --- Cleanup --- |
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.
NIT: This looks like a perfect use case for contextmanager:
from contextlib import contextmanager
@contextmanager
def open_db(path, options):
db = rocksdb.rocksdb_open(options, path.encode())
try:
yield db
finally:
rocksdb.rocksdb_close(db)
@contextmanager
def read_iter(db):
ropts = rocksdb.rocksdb_readoptions_create()
iter_ = rocksdb.rocksdb_create_iterator(db, ropts)
try:
yield iter_
finally:
rocksdb.rocksdb_iter_destroy(iter_)
rocksdb.rocksdb_readoptions_destroy(ropts)
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.
Please consider moving everything from buildkite to scripts folder. Usually we put to buildkite folder stuff which is dependent on buildkite. Looks like it is not a case here
| rocksdb.rocksdb_close(db) | ||
|
|
||
| @contextmanager | ||
| def read_iter(db): |
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.
This should better be a generator, I think. Should leave as a follow-up.
BTW, the reason I'm hand-rolling a binding is because other existing libraries is not using dynamic linking.
|
Superceded by #17957 |
To run this test
install-rocksdb.sh(preferably in a docker because it installs to system library) to ensure rocksdb dyn lib are installedtest.pyinside a venv where everything inrequirements.txtis installed