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

Disable rocksdb concurrent writes #126

Merged
merged 3 commits into from
Jan 27, 2017
Merged

Conversation

yusefnapora
Copy link
Contributor

See mediachain/gorocksdb#1

Basically, the latest rocksdb release (used by the embedded build in gorocksdb as of a few days ago), throws if you try to use OptimizeForPointLookup without setting the allow_concurrent_memtable_write option to false. I added a setter for that option to our fork (and made a PR against the upstream repo: tecbot/gorocksdb#75), and call it from RocksDS.Open to avoid the crash.

I haven't measured how this might impact ingestion performance, but it seems that the unsupported concurrent writes could lead to unpredictable crashes, so I guess it's a good thing to disable them.

@parkan parkan merged commit b878d62 into master Jan 27, 2017
@vyzo
Copy link
Contributor

vyzo commented Jan 30, 2017

This might totally kill ingestion performance, need to test.
Maybe we should disable point lookups instead.

@vyzo
Copy link
Contributor

vyzo commented Jan 30, 2017

the new gcc-6 dependency is also a serious impediment.

@vyzo
Copy link
Contributor

vyzo commented Jan 30, 2017

Doing a test ingestion, the slowdown for parallel ingestion seems to be about 7%.
In the test 4mm ingestion with 8 cpus, total time went up from 9m36.407s to 10m16.377s

Significant, but not a total catastrophe.

@@ -49,6 +49,7 @@ func (ds *RocksDS) Open(home string) error {
}
}
opts.OptimizeForPointLookup(uint64(bcmb))
opts.SetAllowConcurrentMemtableWrites(false)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would very much like to have a comment here.

- gcc-4.8
- g++-4.8
- gcc-6
- g++-6
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't gcc-5 support this?

Copy link
Contributor

Choose a reason for hiding this comment

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

try it out?

Copy link
Contributor

Choose a reason for hiding this comment

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

So it seems to build fine with gcc-5 in the test node, which is a more reasonable compiler for required dependency.

@vyzo vyzo mentioned this pull request Jan 30, 2017
@parkan
Copy link
Contributor

parkan commented Jan 30, 2017

So my impression is that the concurrent writes were never supported, and the upstream change simply made attempting to use them a fatal error whereas it was not previously. It's also possible that the engine used in optimizeForPointLookup (which I tried to track down the code for at some point but gave up after a bit) maybe actually does support it but needs some flags set or smth?

@parkan
Copy link
Contributor

parkan commented Jan 30, 2017

I would say keep it working for the moment and make an issue to investigate this

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.

3 participants