Add eqc trim tests to machi_file_proxy

kuenishi commented

2015-10-30 09:24:26 +00:00

(Migrated from github.com)

QuickCheck took me to the maze of my code.

shino commented

2015-11-04 06:50:14 +00:00

(Migrated from github.com)

csum_table:all_trimmed/3 says

all_trimmed(#machi_csum_table{table=T}, Left, Right) ->

(I guess Left and Right here are both offset (instead of length) only from the variable names)...

And then, file_proxy:handle_call({trim, ...) calls it as

case machi_csum_table:all_trimmed(CsumTable, Offset, Size) of

`csum_table:all_trimmed/3` says ``` all_trimmed(#machi_csum_table{table=T}, Left, Right) -> ``` (I guess `Left` and `Right` here are both offset (instead of length) only from the variable names)... And then, `file_proxy:handle_call({trim, ...)` calls it as ``` case machi_csum_table:all_trimmed(CsumTable, Offset, Size) of ```

shino commented

2015-11-04 08:17:13 +00:00

(Migrated from github.com)

Making testing time to 600 sec, following errors happened.

** Reason for termination ==
** too_many_errors
...
=ERROR REPORT==== 4-Nov-2015::17:12:11 ===
** Too many db tables **

(x10).....(x1)........machi_file_proxy_eqc:41: eqc_test_...*failed*
in function eqc:quickcheck/1 (../src/eqc.erl, line 1276)
in call from machi_file_proxy_eqc:'-eqc_test_/0-fun-1-'/1 (test/machi_file_proxy_eqc.erl, line 41)
**exit:{system_limit,[{ets,new,[machi_csum_table,[private,ordered_set]],[]},

Making testing time to 600 sec, following errors happened. ``` ** Reason for termination == ** too_many_errors ... =ERROR REPORT==== 4-Nov-2015::17:12:11 === ** Too many db tables ** (x10).....(x1)........machi_file_proxy_eqc:41: eqc_test_...*failed* in function eqc:quickcheck/1 (../src/eqc.erl, line 1276) in call from machi_file_proxy_eqc:'-eqc_test_/0-fun-1-'/1 (test/machi_file_proxy_eqc.erl, line 41) **exit:{system_limit,[{ets,new,[machi_csum_table,[private,ordered_set]],[]}, ```

shino commented

2015-11-04 08:45:53 +00:00

(Migrated from github.com)

Changed a parameter to make more contention between planned_writes
and planned_trims as [1]

Then got an error (longer version [2]):

Sequential prefix:

   machi_file_proxy_eqc:start(#state{
     pid = undefined,
     prev_extra = 0,
     planned_writes = [{1025, 4}, {1029, 1}],
     planned_trims = [{1030, 1}],
     written = [{0, 1024}],
     trimmed = []}) ->
       <0.1338.0>
   machi_file_proxy_eqc:write(<0.1338.0>, 1025, {<<0, 0, 0, 0>>, 0, <<>>}) -> ok
   machi_file_proxy_eqc:append(<0.1338.0>, 0,
       {<<0, 0, 0, [snip] 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0>>,
    0, <<>>}) ->
       {ok, "eqc_data7130.627044", 1029}
   machi_file_proxy_eqc:trim(<0.1338.0>, 1030, 1) -> ok
   machi_file_proxy_eqc:write(<0.1338.0>, 1029, {<<0>>, 0, <<>>}) -> {error, written}


Reason:
  Post-condition failed:
  {error, written} /= ok
machi_file_proxy_eqc:40: eqc_test_...*failed*
in function machi_file_proxy_eqc:'-eqc_test_/0-fun-1-'/1 (test/machi_file_proxy_eqc.erl, line 40)
**error:{assertEqual_failed,[{module,machi_file_proxy_eqc},
                     {line,40},
                     {expression,"eqc : quickcheck ( eqc : testing_time ( 15 , ? QC_OUT ( prop_ok ( ) ) ) )"},
                     {expected,true},
                     {value,false}]}

[1] https://gist.github.com/shino/7e77c2fac3af4e3a7c85
[2] https://gist.github.com/shino/ef91a984c5582d09f963

Changed a parameter to make more contention between `planned_writes` and `planned_trims` as [1] Then got an error (longer version [2]): ``` Sequential prefix: machi_file_proxy_eqc:start(#state{ pid = undefined, prev_extra = 0, planned_writes = [{1025, 4}, {1029, 1}], planned_trims = [{1030, 1}], written = [{0, 1024}], trimmed = []}) -> <0.1338.0> machi_file_proxy_eqc:write(<0.1338.0>, 1025, {<<0, 0, 0, 0>>, 0, <<>>}) -> ok machi_file_proxy_eqc:append(<0.1338.0>, 0, {<<0, 0, 0, [snip] 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0>>, 0, <<>>}) -> {ok, "eqc_data7130.627044", 1029} machi_file_proxy_eqc:trim(<0.1338.0>, 1030, 1) -> ok machi_file_proxy_eqc:write(<0.1338.0>, 1029, {<<0>>, 0, <<>>}) -> {error, written} Reason: Post-condition failed: {error, written} /= ok machi_file_proxy_eqc:40: eqc_test_...*failed* in function machi_file_proxy_eqc:'-eqc_test_/0-fun-1-'/1 (test/machi_file_proxy_eqc.erl, line 40) **error:{assertEqual_failed,[{module,machi_file_proxy_eqc}, {line,40}, {expression,"eqc : quickcheck ( eqc : testing_time ( 15 , ? QC_OUT ( prop_ok ( ) ) ) )"}, {expected,true}, {value,false}]} ``` [1] https://gist.github.com/shino/7e77c2fac3af4e3a7c85 [2] https://gist.github.com/shino/ef91a984c5582d09f963

kuenishi commented

2015-11-04 08:53:36 +00:00

(Migrated from github.com)

It looks file_proxy is behaving correctly. I think the verification is wrong.

shino commented

2015-11-04 08:56:59 +00:00

(Migrated from github.com)

Self comment to the above diff. It uses shorter intervals and makes each intervals filled. It will be useful to generate overlapped planned_writes and planned_trims but, downside is it does not have wide variety.

Self comment to the above diff. It uses shorter intervals and makes each intervals filled. It will be useful to generate overlapped `planned_writes` and `planned_trims` but, downside is it does not have wide variety.

jadeallenx commented

2015-11-04 21:52:00 +00:00

(Migrated from github.com)

I think @kuenishi is right - the proxy is behaving correctly and the eqc test is not validating this case properly.

kuenishi commented

2015-11-05 06:03:34 +00:00

(Migrated from github.com)

file_proxy returns {error, written} when a client tried to write to a written chunk (same size and offset, regardless of bytes and checksum), but write_ok believes it's okay if size and offset are exactly same as already written chunk.

file_proxy returns `{error, written}` when a client tried to write to a written chunk (same size and offset, regardless of bytes and checksum), but `write_ok` believes it's okay if size and offset are exactly same as already written chunk.

shino commented

2015-11-05 07:09:04 +00:00

(Migrated from github.com)

Test passed for several runs, diff looks nice 👍
Feel free to merge after one tiny comment above ⛳

Test passed for several runs, diff looks nice :+1: Feel free to merge after one tiny comment above :golf:

shino commented

2015-11-05 07:13:16 +00:00

(Migrated from github.com)

Comments outside of this RP, just for future note, not mandatory for this PR to be merged.

Are filenames in PB high clients string()?
- https://github.com/basho/machi/pull/35/files#diff-84643a086b4e4b105b74001aa044e0ffR130
- https://github.com/basho/machi/blob/master/test/machi_pb_high_client_test.erl#L67-L69
ETS tables owned by file proxy processes are not closed
read_args uses offset(), which likely reterns < 1024

Comments outside of this RP, just for future note, not mandatory for this PR to be merged. - Are filenames in PB high clients string()? - https://github.com/basho/machi/pull/35/files#diff-84643a086b4e4b105b74001aa044e0ffR130 - https://github.com/basho/machi/blob/master/test/machi_pb_high_client_test.erl#L67-L69 - ETS tables owned by file proxy processes are not closed - `read_args` uses `offset()`, which likely reterns < 1024

slfritchie commented

2015-11-06 17:47:10 +00:00

(Migrated from github.com)

@shino "Are filenames in PB high clients string()?" I would like to keep file names as binary() in all code except the PB interface.

@shino "Are filenames in PB high clients string()?" I would like to keep file names as `binary()` in all code except the PB interface.

shino commented

2015-11-07 01:18:43 +00:00

(Migrated from github.com)

@slfritchie May I ask you a question, why use different types?

slfritchie commented

2015-11-08 01:16:27 +00:00

(Migrated from github.com)

Hrm, well, all of the library code is assuming a binary(). But the PB definition file uses "string", which the PB compiler converts to & from list()/string(). Since our conversation a week (?) or so ago in the office about valid char types for filenames, using a bytes type in the PB definition feels to me to be too big

Hrm, well, all of the library code is assuming a binary(). But the PB definition file uses "string", which the PB compiler converts to & from list()/string(). Since our conversation a week (?) or so ago in the office about valid char types for filenames, using a `bytes` type in the PB definition feels to me to be too big

Add eqc trim tests to machi_file_proxy #35