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

Implementation broken #3

Closed
vcastellm opened this issue Nov 21, 2019 · 3 comments
Closed

Implementation broken #3

vcastellm opened this issue Nov 21, 2019 · 3 comments

Comments

@vcastellm
Copy link
Contributor

This implementation is broken using recent versions of https://github.com/hashicorp/raft

Reasoning: On bootstraping raft stores CurrentTerm as the first item in the store, as this implementation doesn't differentiate between conf and logs using prefixes for example, the first call to LastIndex returns the uint64 encoded value of CurrentTerm, therefore it crashes on Raft init.

This implementation is correct https://github.com/markthethomas/raft-badger

@aalda
Copy link
Contributor

aalda commented Nov 22, 2019

You're right Victor but when we implemented this component, it was intended to be used by creating two independent stores for both logs and conf. We could merge them using prefixes, it wouldn't be a big change and probably wouldn't affect performance.

Just to warn you, I've also checked the other implementation and it doesn't seem to deal with Value Log garbage collection, so the method DeleteRange is only deleting the keys in the LSM tree.

Anyways, we finally migrated the Raft's store to RocksDB in the project where we were using this component, so we haven't paid enough attention to this repo since then and it's getting a bit outdated. I can make the changes shortly and maybe upgrade to version 1.6.0 if you're really interested. Upgrading to version 2.0.0 will probably take more time. But, have in mind that we aren't using this project in production anymore so collaboration will be very welcome.

@vcastellm
Copy link
Contributor Author

Thanks @aalda, yes, that's the missing point in the other implementation, the GC. I also know that you migrated to RocksDB, and I wanted to know if it was due to performance problems or other motivation?

I actually migrated it to work with 2.0 but the mentioned issue required more work to be implemented. I can probably take a look to fix it.

@aalda
Copy link
Contributor

aalda commented Nov 22, 2019

Performance was one of the reasons. In fact, we observed an important boost after moving to a vanilla version of RocksDB, but we are aware of that our use case is a bit particular and probably Badger would work well for most workloads. We also needed a storage engine that provided for options to fine tuning important aspects of the LSM tree, the block cache, bloom filters, etc. And also other features like column families. I'm not sure if version 2.0 covers some of these requirements.

@aalda aalda closed this as completed in ee05bc4 Nov 23, 2019
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

No branches or pull requests

2 participants