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

Fixed an issue with some multi_backend versions not matching regular versions #157

Merged
merged 5 commits into from
Mar 27, 2014

Conversation

joedevivo
Copy link
Contributor

Also added a test that will pick problems like that up going forward.

Added a way to include bitcask_multi.schema a la carte. Will have to modify riak and riak_ee to use this, but is a decent compromise for the issue described in basho/eleveldb#80

@joedevivo joedevivo added the Bug label Mar 26, 2014
@joedevivo joedevivo added this to the 2.0-beta milestone Mar 26, 2014
@joedevivo
Copy link
Contributor Author

@jrwest mind a review, bolth of the problem you encountered, and the way this addresses the leveldb issue?

@@ -0,0 +1,39 @@
%% -*- erlang -*-

%% Yo, this is just a test backend for bitcask's multi_backend schema. It's not real
Copy link

Choose a reason for hiding this comment

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

so we would need this in each backend repo? I think I see why but if we change the multi_backend don't we need to change it in 3 other places now...all to prevent changing the multi_backend bitcask/leveldb schemas in 2 places?

Copy link

Choose a reason for hiding this comment

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

i suppose the schemas for the latter case change more often (shrug).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so, this is a contrived test schema for the minimum I need just to prove the multi version of the bitcask schema is working correctly. We will need one of these in the eleveldb repo, but not for memory because memory lives in the riak_kv repo, which has access to the actual multi_backend.schema. This thing shouldn't ever have to change, unless we drastically change how multi_backend is handled, in which case, this test won't be valid anyway.

Copy link

Choose a reason for hiding this comment

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

yea its probably the best of the less ideal cases. Unfortunately, the test also won't fail when its invalid :/

@jrwest
Copy link

jrwest commented Mar 26, 2014

To actually fix the issue I ran into this morning a change in Riak [1] [2] and Riak EE is also necessary.

[1] basho/riak@66666e4
[2] https://github.com/basho/riak/tree/bugfix/jrw/add-bitcask-multi-backend-schema

@joedevivo
Copy link
Contributor Author

@jrwest I'll open a PR on those two repos once the same thing is done for leveldb

@jrwest
Copy link

jrwest commented Mar 26, 2014

+1 merge after the whitespace fixes. fixes bolth the issue I ran into with the improper schema (assuming the Riak build has been modified as described above several times in this PR) and it addresses basho/eleveldb#80 as best it can for now

joedevivo pushed a commit that referenced this pull request Mar 27, 2014
joedevivo added a commit that referenced this pull request Mar 27, 2014
…chema

Fixed an issue with some multi_backend versions not matching regular versions
@joedevivo joedevivo merged commit bae0260 into develop Mar 27, 2014
@joedevivo joedevivo deleted the bugfix/jd/merge-small-file-multi-schema branch March 28, 2014 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants