Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Rename install_hook to set_index_flag #35

Merged
merged 2 commits into from Feb 8, 2013

Conversation

Projects
None yet
2 participants
Contributor

coderoshi commented Jan 30, 2013

Now that all data is indexed (otherwise AAE won't work) there is no longer a notion of a hook. All data written to KV is passed to Yokozuna for minimal indexing. But now there is a flag yz_index_content to indicate to Yokozuna that the content should also be indexed.

  • Rename the code in the appropriate places: yz_kv and yz_wm_index
  • Update documentation, explain that everything is indexed and what the index flag does.

@ghost ghost assigned coderoshi Jan 29, 2013

@rzezeski rzezeski commented on an outdated diff Jan 31, 2013

src/yz_kv.erl
set_index_flag(Bucket, true).
%% @doc Uninstall the object modified hook on the given `Bucket'.
--spec uninstall_hook(binary()) -> ok.
-uninstall_hook(Bucket) when is_binary(Bucket) ->
+-spec unset_index_flag(binary()) -> ok.
+unset_index_flag(Bucket) when is_binary(Bucket) ->
set_index_flag(Bucket, false).
@rzezeski

rzezeski Jan 31, 2013

Contributor
  • Let's get rid of the two functions above and instead use set_index_flag/2 directly.
  • Add a doc explaining that the index flag indicates whether or not the KV object's value should be indexed. If false only the minimal amount of information about the object is indexed so that Yokozuna knows about it and AAE can function correctly. If true then the object's value will be indexed as well.
  • Add spec.

@rzezeski rzezeski commented on the diff Feb 2, 2013

test/yz_kv_tests.erl
@@ -0,0 +1,32 @@
+-module(yz_kv_tests).
+-compile(export_all).
+
+-include("yokozuna.hrl").
+-include_lib("eunit/include/eunit.hrl").
+
+set_index_flag_test()->
@rzezeski

rzezeski Feb 2, 2013

Contributor

This test makes me a bit uneasy. We have had a lot of issues in riak_kv because of tests like this that stand up a limited part of riak. It may work 100% of the time now but in the future as we add more tests, especially more tests that stand up bits of riak, then making sure to fully tear things down becomes very very important. Eunit has non-deterministic run order on different platforms and this has bitten us many times because most of us work on OSX and as tests like these get crufty we start relying on side effects of previous tests (although perhaps we can get around this by spawning nodes for each test, or maybe CT is better at this tuff).

Also, this is a lot of code to test 2 lines of code. Granted, it is important code to test, but if set_index_flag ever breaks it should be obvious (famous last words). Perhaps adding a check to yokozuna_essential that makes sure the bucket flag is set would be wise? In fact, most of this test is really making sure setting bucket properties works. But I believe that is something that should be tested in riak_core and left as a black box in Yokozuna.

All that said, if you feel strongly about this test, we can leave it in. But if it gives us any troubles I will immediately remove it.

Contributor

rzezeski commented Feb 2, 2013

Sorry, I came off like a prick with that last comment. I'm just worried about tests like these given what I've seen in riak_kv over the last 2 years. Yokozuna is a new project. I'd like to try something different this time around. I'd like to keep all eunit and EQC tests in Yokozuna as pure as possible. I.e. only test pure functions or functions that require the most minimal of state setup that is to setup and teardown. I'd like to push all functional testing out to riak_test (this means the EQC test you wrote would have to move into riak_test, or somewhere other than the eunit tests).

I like keeping eunit/EQC pure because

  • it avoids global state (ETS tables, gen_severs, etc) of one test from interfering with another
  • the tests run faster
  • the tests have less chance of randomly breaking thus people actually run them, a lot of people ignore the failing riak_kv tests because they are so unpredictable
  • it's easier to write tests against (almost) pure functions
Contributor

rzezeski commented Feb 8, 2013

+1 to merge

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

Merge pull request #35 from rzezeski/index-flag
Rename install_hook to set_index_flag

@coderoshi coderoshi merged commit fd96f14 into master Feb 8, 2013

1 check failed

default The Travis build could not complete due to an error
Details

@coderoshi coderoshi deleted the index-flag branch Feb 8, 2013

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