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

Feature/list objects eqc addition (squashed) #784

Closed
wants to merge 5 commits into from

Conversation

shino
Copy link
Contributor

@shino shino commented Jan 30, 2014

This PR adds two EQC properties for listing all objects.

  • Two EQC properties for listing objects (GET Bucket),
    one without delimiter and one with delimiter "/".
    Those properties assert following two are equal:

    1. keys from listing objects by repetitive usage of
      riak_cs_list_objects_fsm_v2
    2. active keys from generated [{State, Key},...]
  • When counterexample is found, states and keys are
    written to file $PWD/.riak_cs_list_objects_fsm_v2_eqc.txt.
    Each line consists of line number, status and key.
    Example output:

    1,active,0123456/10
    2,scheduled_delete,0123456/1000
    
  • To generate manifests, generator of whole manifests is avoided
    and generate only states and keys of manifests.
    The property list-all-objects generates several thousands of nested tuples,
    #lfs_manifest_v3 and #acl_v2 in this case.
    But generators of nested tuples seem not so fast.

TODOs remaining (not this PR's TODOs but ones for future)

  • prefix of request parameter
  • non-default max-keys

- Two EQC properties for listing objects (GET Bucket),
  one without delimiter and one with delimiter "/".
  Those properties assert following two are equal:
  1. keys from listing objects by repetitive usage of
     riak_cs_list_objects_fsm_v2
  2. active keys from generated [{State, Key},...]

- When counterexample is found, states and keys are
  written to file $PWD/.riak_cs_list_objects_fsm_v2_eqc.txt.
  Each line consists of line number, status and key.
  Example output:

    1,active,0123456/10
    2,scheduled_delete,0123456/1000

- To generate manifests, generator of whole manifests is avoided
  and generate only states and keys of manifests.
  The property list-all-objects generates several thousands of nested tuples,
  #lfs_manifest_v3 and #acl_v2 in this case.
  But generators of nested tuples seem not so fast.

TODOs remaining
- prefix of request parameter
- non-default max-keys
Clean up the list objects EQC test and start threading the 2i request
page size through spawn_link of the FSM. This allows us to test with
constants far below 1002. 1002 is a nice size for production 2i queries,
but is terrible for getting good shrinks out of QuickCheck.
@reiddraper
Copy link
Contributor

  • non-default max-keys

Added in 0cdb828.

@reiddraper
Copy link
Contributor

This has uncovered a new Riak CS bug. When a delimiter is included, S3 returns an XML node called NextMarker, which is to be used by the client for pagination. Riak CS currently does not support this. Instead, we mistakenly assume the client will just use the last key. Further, when a request is made with both a marker and delimiter, we should be treating the marker as a common delimiter and 'skipping past' it, like we do internally. Once the NextMarker XML node is returned, we should be able to update the EQC test to take advantage of it, and continue running the test. We also may need to update erlcloud, if it doesn't already support this node.

@reiddraper
Copy link
Contributor

Update, erlcloud does not support the NextMarker XML node.

@reiddraper
Copy link
Contributor

I've fixed the lists:last error in 0d2caed, by supporting the NextMarker element. The test is still failing, but it's now failing for a different reason. Progress.

@reiddraper
Copy link
Contributor

Smallest failing case for with_prefix is now something like:

1,writing,ZZZZZZZZ/1
2,writing,ZZZZZZZZ/2
3,active,ZZZZZZZZ/3

@reiddraper
Copy link
Contributor

The bug appears to be related to what @shino initially suspected: we incorrectly skip past a prefix, even if it's deleted. When it's deleted, it won't be included in the result set, and we might be skipping past other keys that fall under that prefix but are not deleted.

@reiddraper
Copy link
Contributor

Removing the skip_past_prefix_and_delimiter optimization makes the test pass. We can't to keep the optimization, but need some way of not using it if the only object that fits under the delimiter is not in the active state.

@reiddraper
Copy link
Contributor

Need to clean things up and squash, but I think I've addressed all of the bugs here. In addition to supporting NextMarker, I've fixed the skip_past_prefix_and_delimiter edge-case here: 3424892.

@shino
Copy link
Contributor Author

shino commented Feb 3, 2014

supporting NextMarker

I cherry-picked eqc part of NextMarker support and pushed some prefix generator modification.

@shino
Copy link
Contributor Author

shino commented Feb 3, 2014

At the current HEAD (ed88f35), I confirmed

  • eqc tests passes with test number 10,000 each (with/without delimiter) with fixes and
  • without those fixes (partly), eqc tests generates conterexamples we faced so far (original 1003 keys, common prefixes but no keys in result, false prefix skipping).

Everything seems nice. I will +1 to that branch after squash! 😂

@reiddraper
Copy link
Contributor

Some of the NextMarker support is missing in the cherry-picking. I'll add it and then re-order the commits.

@shino
Copy link
Contributor Author

shino commented Feb 4, 2014

Close in favor of #788

@shino shino closed this Feb 4, 2014
@shino shino deleted the feature/list-objects-eqc-addition-squashed branch February 17, 2014 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants