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

List objects FSM v2 bugfix and tests #788

Conversation

reiddraper
Copy link
Contributor

Fixes #781.

This is a combination of work from @shino and @reiddraper, intended to fix #781, and add further testing. I also intend for it to replace PR #784, as I have re-ordered some of the commits to ensure Riak CS and the tests compile at each commit. Please compare with the other WIP branches to make sure no changes have been missed.

reiddraper and others added 11 commits February 3, 2014 10:40
- 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.
Don't slice the result-set until `enough_results` has been determined.
Update the list objects FSM and EQC test to support the 'NextMarker' XML
node. This is used when a `delimiter` is included in a request. The
client uses this node to page through the next request, as opposed to
simply using the last `Key` in the contents.
Only use the `skip_past_prefix_and_delimiter` trick if the common
delimiter is already in the result-set. It will _not_ be in the
result-set if the sole object which 'created' it is not in the `active`
state.
@reiddraper
Copy link
Contributor Author

Might be worth getting a third-person (@kellymclaughlin or @andrewjstone) to review, since both @shino and me have put in quite a bit of code.

@andrewjstone
Copy link
Contributor

I can start reviewing this a bit later today. Looks like a good one :)

On Mon, Feb 3, 2014 at 2:50 PM, Reid Draper notifications@github.comwrote:

Might be worth getting a third-person (@kellymclaughlinhttps://github.com/kellymclaughlinor
@andrewjstone https://github.com/andrewjstone) to review, since both
@shino https://github.com/shino and me have put in quite a bit of code.

Reply to this email directly or view it on GitHubhttps://github.com//pull/788#issuecomment-33992537
.

@andrewjstone
Copy link
Contributor

  • manual bucket listing and deletion
  • code inspection
  • eunit and eqc tests
  • xref clean
  • dialyzer clean
  • riak_test

@reiddraper
Copy link
Contributor Author

@shino you mind giving this a once over too to make sure I didn't miss anything?

@@ -271,31 +278,52 @@ enough_results(#state{req=?LOREQ{max_keys=UserMaxKeys},
objects=Objects,
common_prefixes=CommonPrefixes}) ->
riak_cs_list_objects_utils:manifests_and_prefix_length({Objects, CommonPrefixes})
>= UserMaxKeys
> UserMaxKeys
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind add some comments here?
I wonder why there is no equal sign when the name of function says "enough results".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good catch. I'll update in the morning.

@shino
Copy link
Contributor

shino commented Feb 4, 2014

@reiddraper Everything looks nice 💃 . Just a misc comment above.

@andrewjstone
Copy link
Contributor

👍 epic testing

Great work @shino and @reiddraper

reiddraper added a commit that referenced this pull request Feb 4, 2014
…squashed-reid-squashed

List objects FSM v2 bugfix and tests
@reiddraper reiddraper merged commit 8ac7290 into release/1.4 Feb 4, 2014
@reiddraper reiddraper deleted the feature/list-objects-eqc-addition-squashed-reid-squashed branch February 4, 2014 16:33
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

3 participants