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

brick_server new client API - rename #2

Closed
norton opened this issue Feb 4, 2011 · 17 comments
Closed

brick_server new client API - rename #2

norton opened this issue Feb 4, 2011 · 17 comments
Assignees
Milestone

Comments

@norton
Copy link
Contributor

norton commented Feb 4, 2011

Rename a OldKey/Value pair to Key/Value pair in a brick, failing if OldKey does not already exist or if Key already exists. Similar to get_many, the rename API only works for keys of the same chain.

@norton
Copy link
Contributor Author

norton commented Feb 4, 2011

New rename implementation for gdss-brick and gdss-client. Removed deprecated update and append API from gdss-client.

a55c96e
hibari/gdss-client@a8bdb5f

@norton
Copy link
Contributor Author

norton commented Feb 4, 2011

New rename implementation for gdss-admin.

hibari/gdss-admin@96ffa26

@norton
Copy link
Contributor Author

norton commented Feb 9, 2011

Disable server implementation of rename.

5222e90

@norton
Copy link
Contributor Author

norton commented Jul 25, 2011

Prototype for this change request is on branch norton-server-rename.

@tatsuya6502
Copy link
Member

I tried the prototype and found one issue. If I don't use a binary but string for the new key, the key value will be lost. I'll look after this issue.

@tatsuya6502
Copy link
Member

Updated QuickCheck (qc) to detect the issue above. hibari/gdss-admin@d101a50

To run qc, the repo manifest has to be updated. I'll do so after work.

@tatsuya6502
Copy link
Member

I've been doing source code reivew and checked the write path for rename operation. I didn't find any obvious place to cause the issue on 7th comment.

  • I have to revise QuickCheck test case. The current one assumes a key of string or iodata type is serialized to binary when it's stored in brick. This is wrong; the key is stored as is, so key <<"a">> and "a" are treated as different keys in Hibari.
  • Even if QuickCheck case is invalid, I'm sure the issue exists. I originaly found the issue by a local eunit case, which only uses string as the keys sharing the same prifix. I verified not only get operation can't find the new key after the rename but also get_many operation to scan inside the chain returned [].

I think possible cause of the issue might be brick_ets:my_insert_ignore_logging/3 for insert_constant_value (?VALUE_SWITCHAROO) is not executed or silently fails if the new key is not binary. But how?

I'm going to enable the tracepoints to see the rename operation in action.

@tatsuya6502
Copy link
Member

I fixed the issue described in 7th comment (#2 (comment)) and pushed the changes to norton-server-rename branch.

Commit: fe1091c

Cause:
In brick_server:make_rename/2, the new key is not converted to binary while old key is done so. brick_server:make_op5 and make_op6 apply gmt_util:bin_ify/1 only to the old key, and brick_ets will silently fail to insert a key value if its key is not a binary.

Note: I was wrong in 10th comment (#2 (comment)). Keys are not stored "as is", but converted to binary just as I first thought. So my QuickCheck cases were actually correct.

Verification
Ran the QuickCheck cases in simple_eqc_tests and verified that they passed 2,000 tests.

(hibariclient@127.0.0.1)2> simple_eqc_tests:run(2000).
prop_simple1:
...
Hibari: get_many() has been commented out from command()

.c......v....Yv....c.vvv.Y....vYY..v...c...cc.......Y.c.
...
OK, passed 2000 tests

51.05% {0,div10}
23.20% {1,div10}
12.45% {2,div10}
6.55% {3,div10}
3.45% {4,div10}
1.40% {5,div10}
0.75% {6,div10}
0.35% {7,div10}
0.30% {8,div10}
0.15% {11,div10}
0.15% {9,div10}
0.10% {10,div10}
0.05% {14,div10}
0.05% {13,div10}


prop_simple1_noproc_ok:
...
Hibari: get_many() has been commented out from command()

v.c...Y.v..v............vvcv.c..c.cY..v.c.Y.......Y..v..
...
OK, passed 2000 tests

50.60% {0,div10}
24.20% {1,div10}
12.05% {2,div10}
6.55% {3,div10}
2.75% {4,div10}
1.80% {5,div10}
1.05% {6,div10}
0.40% {7,div10}
0.35% {8,div10}
0.10% {11,div10}
0.10% {9,div10}
0.05% {10,div10}
[]

@tatsuya6502
Copy link
Member

Merged into the dev branch:
#10 (comment)

@tatsuya6502
Copy link
Member

@tatsuya6502 tatsuya6502 reopened this Mar 31, 2013
@tatsuya6502
Copy link
Member

I decided the following things:

  1. As for the hibari issue 33 (bad sequence file 'enoent' error), I will use the solution described in this comment of the issue, which introduces a concept of frozen hlog. Right now, I have a prototype implementation with squidflash-primer working for a frozen hlog only on a head brick. I will continue work on it and implement squidflash-primer on downstream bricks (middle and official/non-official tail bricks) as well.
  2. I am going to replace rename/6 on brick server with copy/6, which is one-step simpler than rename and can be used as a building block of rename.
  3. I am going to add rename/6 on the client API (brick_simple). It is just a convenient function to do do([make_txn(), make_copy(OldKey, NewKey, ...), make_delete(OldKey, ...)], ...).

Target milestone is still v0.3.0.

(Edit)
Here is the link to GH issue of copy/6.

@tatsuya6502
Copy link
Member

  1. As for the hibari issue 33 (bad sequence file 'enoent' error),
    ...

    Right now, I have a prototype implementation with
    squidflash-primer working for a frozen hlog only on a head
    brick
    . I will continue work on it and implement
    squidflash-primer on downstream bricks (middle and official/
    non-official tail bricks) as well.

I made a prototype of this (commit) but then realized this breaks modification replay in downstream bricks because it changes the order of modifications by delaying rename operation. I decided not to do squidflash-priming in downstream bricks. (I reverted the commit in the very next commit.)

@tatsuya6502
Copy link
Member

I have decided to include this feature (rename/6) into upcoming Hibari v0.1.11 release. I modified the codes on the dev branch so that brick_ets will act like hlog sequences are always frozen. I'll revisit this in next unstable major release v0.3.

Commit

gdss-brick >> GH2 - brick_server new client API - rename

rename/6 operation now always uses rename_key_with_get_set_delete/6 instead of my_insert/6 with ?KEY_SWITCHAPOO. Will revisit this in Hibari v0.3 release.

@tatsuya6502
Copy link
Member

Changing the milestone from v0.3 to v0.1.11.

@tatsuya6502
Copy link
Member

Commit

gdss-brick >> GH2 - brick_server new client API - rename

rename/6 operation now always uses rename_key_with_get_set_delete/6 instead of my_insert/6 with ?KEY_SWITCHAPOO. Will revisit this in Hibari v0.3 release.

QuickCheck property prop_simple1 has found a bug in this implementation. If new key is overwriting an existing key-value with {exp_time_directive, keep} and {attrib_directive, keep}, rename_key_with_get_set_delete/6 will not only keep the exp_time and flags from old key (correct), but also from the existing key-value (wrong).

[{set,{var,1},
      {call,simple_eqc_tests,simple_set,
            [["/foo/bar/",["000",50]],
             <<"v a">>,1426323338786789025,
             [{exp_time_directive,keep}],
             [{attrib2,"val2"}]]}},
 {set,{var,2},
      {call,simple_eqc_tests,simple_set,
            [<<"/foo/bar/0001">>,<<"v a">>,1426323338789594039,
             [{exp_time_directive,replace},{attrib_directive,keep}],
             []]}},
 {set,{var,3},
      {call,simple_eqc_tests,simple_rename,
            [<<"/foo/bar/0001">>,
             ["/foo/bar/",["000",50]],
             1426323338789723040,
             [{exp_time_directive,keep},{attrib_directive,keep}],
             []]}},
 {set,{var,4},
      {call,simple_eqc_tests,simple_get,["/foo/bar/0002",[get_all_attribs]]}}]
S = {state,5,
           [{<<"/foo/bar/0001">>,undefined},
            {<<"/foo/bar/0002">>,1426323338789594039},
            {<<"/foo/bar/0003">>,undefined},
            {<<"/foo/bar/0004">>,undefined}],
           [{<<"/foo/bar/0001">>,[]},
            {<<"/foo/bar/0002">>,[<<"v a">>]},
            {<<"/foo/bar/0003">>,[]},
            {<<"/foo/bar/0004">>,[]}],
           [{<<"/foo/bar/0001">>,undefined},
            {<<"/foo/bar/0002">>,[]},
            {<<"/foo/bar/0003">>,undefined},
            {<<"/foo/bar/0004">>,undefined}],
           [{tab1_ch1_b1,gdss_eunit@localhost}]}
R = {postcondition,false}
Hist = [{eqc_statem_history,
            {state,1,
                [{<<"/foo/bar/0001">>,undefined},
                 {<<"/foo/bar/0002">>,undefined},
                 {<<"/foo/bar/0003">>,undefined},
                 {<<"/foo/bar/0004">>,undefined}],
                [{<<"/foo/bar/0001">>,[]},
                 {<<"/foo/bar/0002">>,[]},
                 {<<"/foo/bar/0003">>,[]},
                 {<<"/foo/bar/0004">>,[]}],
                [{<<"/foo/bar/0001">>,undefined},
                 {<<"/foo/bar/0002">>,undefined},
                 {<<"/foo/bar/0003">>,undefined},
                 {<<"/foo/bar/0004">>,undefined}],
                [{tab1_ch1_b1,gdss_eunit@localhost}]},
            {call,simple_eqc_tests,simple_set,
                [["/foo/bar/",["000",50]],
                 <<"v a">>,1426323338786789025,
                 [{exp_time_directive,keep}],
                 [{attrib2,"val2"}]]},
            [],
            {normal,{ok,1426323340565845}}},
        {eqc_statem_history,
            {state,2,
                [{<<"/foo/bar/0001">>,undefined},
                 {<<"/foo/bar/0002">>,1426323338786789025},
                 {<<"/foo/bar/0003">>,undefined},
                 {<<"/foo/bar/0004">>,undefined}],
                [{<<"/foo/bar/0001">>,[]},
                 {<<"/foo/bar/0002">>,[<<"v a">>]},
                 {<<"/foo/bar/0003">>,[]},
                 {<<"/foo/bar/0004">>,[]}],
                [{<<"/foo/bar/0001">>,undefined},
                 {<<"/foo/bar/0002">>,[{attrib2,"val2"}]},
                 {<<"/foo/bar/0003">>,undefined},
                 {<<"/foo/bar/0004">>,undefined}],
                [{tab1_ch1_b1,gdss_eunit@localhost}]},
            {call,simple_eqc_tests,simple_set,
                [<<"/foo/bar/0001">>,<<"v a">>,1426323338789594039,
                 [{exp_time_directive,replace},{attrib_directive,keep}],
                 []]},
            [],
            {normal,{ok,1426323340572921}}},
        {eqc_statem_history,
            {state,3,
                [{<<"/foo/bar/0001">>,1426323338789594039},
                 {<<"/foo/bar/0002">>,1426323338786789025},
                 {<<"/foo/bar/0003">>,undefined},
                 {<<"/foo/bar/0004">>,undefined}],
                [{<<"/foo/bar/0001">>,[<<"v a">>]},
                 {<<"/foo/bar/0002">>,[<<"v a">>]},
                 {<<"/foo/bar/0003">>,[]},
                 {<<"/foo/bar/0004">>,[]}],
                [{<<"/foo/bar/0001">>,[]},
                 {<<"/foo/bar/0002">>,[{attrib2,"val2"}]},
                 {<<"/foo/bar/0003">>,undefined},
                 {<<"/foo/bar/0004">>,undefined}],
                [{tab1_ch1_b1,gdss_eunit@localhost}]},
            {call,simple_eqc_tests,simple_rename,
                [<<"/foo/bar/0001">>,
                 ["/foo/bar/",["000",50]],
                 1426323338789723040,
                 [{exp_time_directive,keep},{attrib_directive,keep}],
                 []]},
            [],
            {normal,{ok,1426323340580630}}},
        {eqc_statem_history,
            {state,4,
                [{<<"/foo/bar/0001">>,undefined},
                 {<<"/foo/bar/0002">>,1426323338789594039},
                 {<<"/foo/bar/0003">>,undefined},
                 {<<"/foo/bar/0004">>,undefined}],
                [{<<"/foo/bar/0001">>,[]},
                 {<<"/foo/bar/0002">>,[<<"v a">>]},
                 {<<"/foo/bar/0003">>,[]},
                 {<<"/foo/bar/0004">>,[]}],
                [{<<"/foo/bar/0001">>,undefined},
                 {<<"/foo/bar/0002">>,[]},
                 {<<"/foo/bar/0003">>,undefined},
                 {<<"/foo/bar/0004">>,undefined}],
                [{tab1_ch1_b1,gdss_eunit@localhost}]},
            {call,simple_eqc_tests,simple_get,
                ["/foo/bar/0002",[get_all_attribs]]},
            [],
            {normal,
                {ok,1426323340580630,<<"v a">>,1426323338786789025,
                    [{val_len,3},{attrib2,"val2"}]}}}]

@tatsuya6502
Copy link
Member

QuickCheck property prop_simple1 has found a bug in this implementation. If new key is overwriting an existing key-value with {exp_time_directive, keep} and {attrib_directive, keep}, rename_key_with_get_set_delete/6 will not only keep the exp_time and flags from old key (correct), but also from the existing key-value (wrong).

Fixed this by 8c7fb13.
Now prop_simple1 passes 2,000 tests.

Erlang/OTP 17 [erts-6.3] [source] [64-bit] [smp:8:8] [async-threads:64] [hipe] [kernel-poll:true]

Eshell V6.3  (abort with ^G)
(hibariclient@127.0.0.1)1> eqc:quickcheck(eqc:numtests(2000, simple_eqc_tests:prop_simple1())).


Hibari: get_many() has been commented out from command()

Starting Quviq QuickCheck version 1.33.3
   (compiled at {{2015,2,9},{14,18,24}})
...
vY.YY.Yv....c.....c....Y.......cv.Y.v..c.Y....v....Y.v......c.c..
...
OK, passed 2000 tests

49.45% {0,div10}
25.00% {1,div10}
12.20% {2,div10}
5.75% {3,div10}
3.05% {4,div10}
2.00% {5,div10}
0.85% {6,div10}
0.80% {7,div10}
0.40% {9,div10}
0.20% {10,div10}
0.20% {8,div10}
0.05% {16,div10}
0.05% {11,div10}
true
(hibariclient@127.0.0.1)2> 

@tatsuya6502
Copy link
Member

Fixed this by 8c7fb13.
Now prop_simple1 passes 2,000 tests.

Applied the same fix to branch gbrick-gh17-redesign-disk-storage: e9957c5

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

No branches or pull requests

2 participants