Intermittent test failure fixes #1657
Conversation
The `gen_valid_bucket` call used regular `gen_hll_precision` meaning that every now and again the _valid_ bucket had an _invalid_ hll property. When this coincides with the update property also having the same invalid hll property the resulting bucket has an invalid hll property that matches the input and so it appears that validation failed, and rightly the test fails. However validation worked, the original bucket was invalid is all. This commit adds a `gen_valid_hll_precision` generator so that the original "valid" bucket is actually valid.
A duplicate name was is used for the get_fsm_active stat, and also the registration code is not robust. This commit removes the unused duplicate alias and makes the registration code more robust by using re-register.
Due to bad assumptions about paths to deps schema tests and cli tests failed reliably from a top level checkout. This commit, while not pretty or exact passes of both top level checkouts and local deps checkouts. I don't think it is worth buring more time making it better or less repetitive unless you do?
Cuttlefish will freakout and die and refuse to start a riak node if there is an unknown/mapped property in riak.conf. I guess this is good because...anyway, this is a speculative workaround for the upgrade from riak_ee to riak+repl, since there is likely a `jmx` config setting in riak.conf. Probably the right answer is to port riak_jmx to riak-2.2.5 and deprecate it.
| @@ -516,7 +516,7 @@ stats() -> | |||
| %% node stats: gets | |||
| {[node, gets], spiral, [], [{one , node_gets}, | |||
| {count, node_gets_total}]}, | |||
| {[node, gets, fsm, active], counter, [], [{value, node_get_fsm_active}]}, | |||
| {[node, gets, fsm, active], counter}, | |||
There was a problem hiding this comment.
Looks good, just a question really, how come the other args are no longer required?
There was a problem hiding this comment.
No idea, it looks wrong though, eh? node_get_fsm_active vs node_gets everywhere else. Damn, I wish I could remember october 2017 a little better.
There was a problem hiding this comment.
Hmmm…yeah…really…no idea…will have to go back and read through it all again
There was a problem hiding this comment.
I honestly don't understand stats since they were changed to exometer.
There was a problem hiding this comment.
Oh the commit message says it is "an unused duplicate alias", that answer your question?
There was a problem hiding this comment.
that's fine, just curious
There was a problem hiding this comment.
Look at
Line 622 in 35b6e56
src/riak_kv_test_util.erl
Outdated
| %% F(start), | ||
| %% Acc | ||
| %% end, | ||
| [], Apps); |
There was a problem hiding this comment.
Good call - nested function definition inside fold was ugly.
There was a problem hiding this comment.
should probably fix the commenting out though
src/riak_kv_test_util.erl
Outdated
| @@ -265,8 +264,9 @@ dep_apps(Test, Extra) -> | |||
| application:set_env(riak_core, ring_state_dir, Test ++ "/ring"), | |||
| application:set_env(riak_core, platform_data_dir, Test ++ "/data"), | |||
| application:set_env(riak_core, handoff_port, 0), %% pick a random handoff port | |||
| {ok, Dir} = file:get_cwd(), | |||
| Dirs = [Dir ++ "/../deps/*/priv"], | |||
| %% @TODO this is wrong still | |||
There was a problem hiding this comment.
some indication of why it's wrong might be helpful
There was a problem hiding this comment.
…erm…you got me. It's "wrong" in that it is a hack maybe?
There was a problem hiding this comment.
Adding a comment to draw attention to it doesn't detract from the P/R - don't pay this one any more attention on my account.
src/riak_kv_test_util.erl
Outdated
| @@ -326,6 +339,7 @@ start_app_and_deps(Application, Started) -> | |||
| true -> | |||
| {ok, Started}; | |||
| false -> | |||
| _Apps = application:which_applications(), | |||
There was a problem hiding this comment.
I'm curious as to what this does
There was a problem hiding this comment.
I have no idea…I'll have to have a read of the whole file again. This is what happens with so much time between commit and review, eh?
There was a problem hiding this comment.
I think it just needs to be taken out, probably a different idea, and then underbarred the binding just to make it compile.
There was a problem hiding this comment.
Oh yeah, it was for some printf debugging
You don't need it!
`Apps` was used for a some debugging
|
Pushed some commits to address @bryanhuntesl comments |
It's fixed in riak_test upgrade test code instead. No need to pollute future riak releases with any jmx carry overs.
When the 2.2.5 PRs are merged this rebar.config is correct for the set of deps for 2.2
|
Same question, squash first? |
|
+1 squash |
Also some deps changes that will need looking at again as we move from tags to branches.
Still for discussion - the JMX stuff in the schema. It should probably be complete for all jmx properties, and the same for SNMP, and an error logged at start up if they're configured
trueoron.The other commits are test fixes for
hllandstats, and some dirty path munging for running tests either in riak_kv or at the top level riak project.