Skip to content
This repository has been archived by the owner on Apr 30, 2019. It is now read-only.

Update to rocksbd-5.3.6 to fix a problem in the rocksdb jni wrapper #9

Merged
merged 1 commit into from
Jun 17, 2017

Conversation

merlimat
Copy link
Collaborator

No description provided.

Copy link

@rdhabalia rdhabalia left a comment

Choose a reason for hiding this comment

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

👍

@rdhabalia rdhabalia merged commit 07b2205 into yahoo-4.3 Jun 17, 2017
@merlimat merlimat deleted the merlimat-patch-1 branch June 17, 2017 00:40
@revans2
Copy link
Collaborator

revans2 commented Jun 20, 2017

@merlimat you have a lot more experience with rocksdb than I do. Could you take a look at facebook/rocksdb#2472? I found it while trying to make a release with this change in it and it scares me enough that I am tempted to revert this pull request, but I wanted to hear from you first.

@merlimat
Copy link
Collaborator Author

@revans2 The change you mentioned in the rocksdb issue it's very large and it's difficult to understand all the implications of it.

The only difference I see, when running with DEBUG=0 is that is not passing the -Xcheck:jni to the JVM, though I don't understand how could that "prevent" the error from happening.

The other possibility would be some bug in the JNI wrapper that only shows up when the optimizer is enabled.

Have you guys seen any problem related to this when running the bookies?
I can run some tests locally as well to try to debug it.

@revans2
Copy link
Collaborator

revans2 commented Jun 20, 2017

@merlimat We have not moved to 5.3.6 yet. I found the issue as we were preparing to do so, so I cannot say anything about production.

The only other difference I know of is that -Werror and -DNDEBUG are added to the compilation command line. The optimization levels are the same in each -02. I was curious if you had been using this at all, and if you had done much with testing the new functionality?

@merlimat
Copy link
Collaborator Author

I only did some test some time back (with rocksdb-5.3.4) when I added the deleteRange to DbLedgerStorage. I was about to get started testing with the last 1.18 release bookies and brokers in the next few days.

@merlimat
Copy link
Collaborator Author

@revans2 One thing from your comment: you said the problem was "apparently" fixed in master in a commit that reordered the fields in ReadOptions and in one of the test the corruption was happening when doing the ReadOptionsTests in jni, so maybe the 2 things are really correlated.

@merlimat
Copy link
Collaborator Author

@msb-at-yahoo might have some idea on how the reordering might have "fixed" the issue.

@revans2
Copy link
Collaborator

revans2 commented Jun 21, 2017

@merlimat I figured out what the issue is. It looks like an assertion is providing some kind of protection and DEBUG=0 disables that assertion which lets a test do bad things.

No need to revert things, but it might be good going forward to run bookies with -ea just to be on the safe side.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants