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

Consider removing StorageEmbeddedRocksDB. #18655

Closed
alexey-milovidov opened this issue Dec 30, 2020 · 11 comments
Closed

Consider removing StorageEmbeddedRocksDB. #18655

alexey-milovidov opened this issue Dec 30, 2020 · 11 comments
Labels
feature help wanted st-discussion The story requires discussion /research / expert help / design & decomposition before will be taken

Comments

@alexey-milovidov
Copy link
Member

Bugs have been found:

facebook/rocksdb#7711
facebook/rocksdb#7821
facebook/rocksdb#7778

Are there use cases that will motivate against removal?

@chenziliang
Copy link
Contributor

We are using rocksdb table engine as a kv store for now since we don't wanna introduce another kv store dependency. If we get this removed, may i ask if there is any other alternative in CH (ReplacingMergeTree etc may not be as convenient as kv store) ?

@alexey-milovidov
Copy link
Member Author

@chenziliang Thank you! Now the chance it will be removed is low.

@alexey-milovidov alexey-milovidov added the st-discussion The story requires discussion /research / expert help / design & decomposition before will be taken label Jan 1, 2021
@alexey-milovidov
Copy link
Member Author

But we have to fix bugs, otherwise it will make constant noise in our CI system.

@alexey-milovidov
Copy link
Member Author

@sundy-li Maybe you want to help fixing the remaining use-after-free error? facebook/rocksdb#7821

@sundy-li
Copy link
Contributor

sundy-li commented Jan 4, 2021

@sundy-li Maybe you want to help fixing the remaining use-after-free error? facebook/rocksdb#7821

Sure.

BTW I was not aware that rocksdb may introduce lots of bugs to broke ClickHouse. Because I thought there were many newsql database built base on rocksdb (cockroachdb, tidb, yugabytedb), it proves that ClickHouse has better tests systems than other databases . I am not rocksdb expert, but I'm willing to help it work better in ClickHouse.

@alexey-milovidov
Copy link
Member Author

cockroachdb, tidb, yugabytedb

Maybe none of them have CI testing with fuzzing and all four sanitizers?
Or they test only frontend part of the codebase but don't test RocksDB itself?

@alexey-milovidov
Copy link
Member Author

All bugs has been fixed.

@toktarev
Copy link

Not sure if it is correct to write here, but curiosity and this is Open Source :).

I have a question to ClickHouse team regarding usage of RocksDB.

Guys, have you faced with RocksDB's memory problems ?

I mean unlimited memory growth during long period of insertion ?

I receive more and more complains during last 3 years in this issue: facebook/rocksdb#4112.

@alexey-milovidov
Copy link
Member Author

@toktarev It's not actively used yet and we have no reports about this issue.

There are no tests for long period of insertion - maximum are fuzz and stress tests that last only one hour (for every commit).
And that tests are not specific - they run queries with RocksDB only occasionally (by random choice among other tests).
If memory won't grow for several tens of GB in short period of time - these tests will not notice.

There is still a chance that we will notice something if it exists...

@toktarev
Copy link

@alexey-milovidov , I see , thanks.

@alexey-milovidov
Copy link
Member Author

#21479

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature help wanted st-discussion The story requires discussion /research / expert help / design & decomposition before will be taken
Projects
None yet
Development

No branches or pull requests

4 participants