Add eqc trim tests to machi_file_proxy

[kuenishi]

QuickCheck took me to the maze of my code.

[shino]

It seems some built-in types are defined in singular, e.g. atom(), term(), boolean(). Which is better...?

[kuenishi]

I think singular is better, too.

[shino]

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]

offset() returns small positive integers which probably inside 1024-byte header.

[kuenishi]

Addressed in f560372.

[shino]

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]

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]

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

[shino]

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]

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

[kuenishi]

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]

probably_error/1 is not used.

[shino]

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

[shino]

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]

@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]

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

[slfritchie]

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