Files Overwritten on Core Failure #14

Merged
merged 1 commit into from Feb 1, 2013

Conversation

Projects
None yet
2 participants
@coderoshi
Contributor

coderoshi commented Jan 29, 2013

If a Solr Core fails to initialize, or is not up for any reason, and a
ring event comes in then Yokozuna will try to create it because
yz_index:exists returns false if the Core is not running. In most
cases this is harmless because the index data will not be lost. But
in the case where modifications have been made to files living under
the instanceDir they will be lost. This is because Yokozuna will
re-copy the configuration files overwriting any modifications made to
those local to the instance.

Reproduce

  • Run against Yokozuna commit 448fe02b1691b7943dcbdc87df4074d1b1fbde4a
  • Build a Riak rel, make stage, and start it
  • Create a new index yz_index:create("foo")
  • Stop Riak
  • Edit rel/riak/data/yz/foo/conf/config.xml and remove the
    startup="lazy" from the ExtractingRequestHandler.
  • Start Riak and tail -f rel/riak/log/solr-0.log
  • You'll see a failure to load the class and then shortly after a
    successful creation of the core
  • Reopen rel/riak/data/yz/foo/conf/config.xml and you'll see the
    startup="lazy" is there again.

Potential Solutions

  • Have yz_index:exists distinguish between a downed Solr Core and
    one that doesn't exist at all. Take different actions depending on
    the situation.
  • During file copy only overwrite files that don't already exist.
  • Same as previous but only overwrite if the source file is newer.

@ghost ghost assigned coderoshi Jan 8, 2013

+copy_files([File|Rest], Dir) ->
+ copy_files([File|Rest], Dir, overwrite).
+
+%% If destination exists, skip the copy

This comment has been minimized.

Show comment Hide comment
@rzezeski

rzezeski Jan 31, 2013

Contributor

Given this function is a predicate I think it would be better named should_copy.

@rzezeski

rzezeski Jan 31, 2013

Contributor

Given this function is a predicate I think it would be better named should_copy.

@rzezeski

View changes

test/yz_misc_tests.erl
+
+-include_lib("eunit/include/eunit.hrl").
+
+prop_does_copy_action_skip_test() ->

This comment has been minimized.

Show comment Hide comment
@rzezeski

rzezeski Jan 31, 2013

Contributor

Might want to consider removing the prop_ prefix. In all the Riak code I've seen we use prop_ to indicate an EQC test.

@rzezeski

rzezeski Jan 31, 2013

Contributor

Might want to consider removing the prop_ prefix. In all the Riak code I've seen we use prop_ to indicate an EQC test.

This comment has been minimized.

Show comment Hide comment
@coderoshi

coderoshi Jan 31, 2013

Contributor

will do. case of repurposing, this started life as an eqc test

@coderoshi

coderoshi Jan 31, 2013

Contributor

will do. case of repurposing, this started life as an eqc test

@rzezeski

View changes

test/yz_misc_tests.erl
+
+prop_does_copy_action_skip_test() ->
+ Thisfile = atom_to_list(?MODULE) ++ ".erl",
+ false = yz_misc:does_copy_action(skip, Thisfile, Thisfile),

This comment has been minimized.

Show comment Hide comment
@rzezeski

rzezeski Jan 31, 2013

Contributor

At the end of the day this probably doesn't matter so much, but if you use the eunit macros such as ?assert and ?assertNot the error message is a bit more descriptive. Feel free to stick with matching, but thought I'd point this out in case you haven't seen it.

Matching

in function yz_misc_tests:prop_does_copy_action_overwrite_test/0 (test/yz_misc_tests.erl, line 58)
**error:{badmatch,true}

?assertNot

    yz_misc_tests: prop_does_copy_action_overwrite_test...*failed*
in function yz_misc_tests:'-prop_does_copy_action_overwrite_test/0-fun-0-'/0 (test/yz_misc_tests.erl, line 58)
in call from yz_misc_tests:prop_does_copy_action_overwrite_test/0 (test/yz_misc_tests.erl, line 58)
**error:{assertion_failed,[{module,yz_misc_tests},
                   {line,58},
                   {expression,"not ( yz_misc : does_copy_action ( overwrite , \"<nofile>\" , \"<nofile>\" ) )"},
                   {expected,true},
                   {value,false}]}
@rzezeski

rzezeski Jan 31, 2013

Contributor

At the end of the day this probably doesn't matter so much, but if you use the eunit macros such as ?assert and ?assertNot the error message is a bit more descriptive. Feel free to stick with matching, but thought I'd point this out in case you haven't seen it.

Matching

in function yz_misc_tests:prop_does_copy_action_overwrite_test/0 (test/yz_misc_tests.erl, line 58)
**error:{badmatch,true}

?assertNot

    yz_misc_tests: prop_does_copy_action_overwrite_test...*failed*
in function yz_misc_tests:'-prop_does_copy_action_overwrite_test/0-fun-0-'/0 (test/yz_misc_tests.erl, line 58)
in call from yz_misc_tests:prop_does_copy_action_overwrite_test/0 (test/yz_misc_tests.erl, line 58)
**error:{assertion_failed,[{module,yz_misc_tests},
                   {line,58},
                   {expression,"not ( yz_misc : does_copy_action ( overwrite , \"<nofile>\" , \"<nofile>\" ) )"},
                   {expected,true},
                   {value,false}]}
@rzezeski

This comment has been minimized.

Show comment Hide comment
@rzezeski

rzezeski Jan 31, 2013

Contributor
  • Compiles + eunit passes
  • No new dialyzer errors
  • Verified it does not overwrite core
Contributor

rzezeski commented Jan 31, 2013

  • Compiles + eunit passes
  • No new dialyzer errors
  • Verified it does not overwrite core
@rzezeski

This comment has been minimized.

Show comment Hide comment
@rzezeski

rzezeski Feb 1, 2013

Contributor

Looks like you accidentally added the log dir.

Contributor

rzezeski commented Feb 1, 2013

Looks like you accidentally added the log dir.

@rzezeski

This comment has been minimized.

Show comment Hide comment
@rzezeski

rzezeski Feb 1, 2013

Contributor

+1 to merge

Contributor

rzezeski commented Feb 1, 2013

+1 to merge

coderoshi added a commit that referenced this pull request Feb 1, 2013

@coderoshi coderoshi merged commit 6496835 into master Feb 1, 2013

@coderoshi coderoshi deleted the core-overwrite-safety branch Feb 1, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment