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

storage: add basic mz-rocksdb crate #18770

Merged
merged 3 commits into from
Apr 18, 2023
Merged

Conversation

guswynn
Copy link
Contributor

@guswynn guswynn commented Apr 13, 2023

This pr is in-service of #17067.

It adds a mz-rocksdb crate, derived from this benchmark: https://github.com/MaterializeInc/materialize/blob/main/src/storage/examples/upsert_open_loop.rs, and unit tests it. It is async (so it is easy to integrate into timely dataflows, and runs the rocksdb instance

@danhhz for the metrics, I opted for adding some rocksdb specific ones (latencies of gets and writes), and will get other metrics (like e2e latency for a full batch upsert) using the trait method we discussed, in later pr's. Let me know if you spot any other metrics that make sense!

Motivation

  • This PR adds a known-desirable feature.

Tips for reviewer

Some amount of complexity here is due to graceful shutdown being required by rocksdb: rust-rocksdb/rust-rocksdb#767

Unfortunately, it seems as rocksdb is determined to segfault in scenarios we wont be able to gracefully shutdown from.

Note also that rocksdb errors (like io errors) are considered permanently fatal, and in the future, will cause sources to restart

Checklist

src/rocksdb/src/lib.rs Outdated Show resolved Hide resolved
src/rocksdb/src/lib.rs Outdated Show resolved Hide resolved
src/rocksdb/src/lib.rs Outdated Show resolved Hide resolved
src/rocksdb/src/lib.rs Outdated Show resolved Hide resolved
src/rocksdb/src/lib.rs Show resolved Hide resolved
src/rocksdb/src/lib.rs Outdated Show resolved Hide resolved
src/rocksdb/src/lib.rs Outdated Show resolved Hide resolved
src/rocksdb/src/lib.rs Outdated Show resolved Hide resolved
src/rocksdb/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@aljoscha aljoscha left a comment

Choose a reason for hiding this comment

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

Looks very good! Modulo some stray comments/copy-paste errors, and some minor restructuring of, say, tests.

Did you try slotting this into the upsert open-loop benchmark?

src/rocksdb/src/lib.rs Outdated Show resolved Hide resolved
src/rocksdb/src/lib.rs Outdated Show resolved Hide resolved
src/rocksdb/src/lib.rs Outdated Show resolved Hide resolved
src/rocksdb/src/lib.rs Outdated Show resolved Hide resolved
src/rocksdb/src/lib.rs Show resolved Hide resolved
src/rocksdb/src/lib.rs Outdated Show resolved Hide resolved
src/rocksdb/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@danhhz danhhz left a comment

Choose a reason for hiding this comment

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

overall looks good! i'll leave the stamp to aljoscha. definitely include me when you PR the pluggable trait for this, please

src/rocksdb/src/lib.rs Outdated Show resolved Hide resolved
src/rocksdb/src/lib.rs Outdated Show resolved Hide resolved
src/rocksdb/src/lib.rs Show resolved Hide resolved
src/rocksdb/src/lib.rs Outdated Show resolved Hide resolved
src/rocksdb/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@philip-stoev philip-stoev left a comment

Choose a reason for hiding this comment

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

Not sure what is there for me to test here, but please do add me to any PR that implements the actual spill to disk!

@aljoscha aljoscha self-requested a review April 18, 2023 07:40
@guswynn guswynn merged commit 825027b into MaterializeInc:main Apr 18, 2023
@guswynn guswynn mentioned this pull request Apr 24, 2023
34 tasks
@guswynn guswynn deleted the mz-rocksdb branch May 26, 2023 18:19
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.

None yet

5 participants