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

Bugfix: prune user buckets #812

Merged
merged 5 commits into from
Feb 20, 2014
Merged

Conversation

reiddraper
Copy link
Contributor

Fixes #798.

This was previously hardcoded to 24-hours. This allows the pruning time
to be runtime (and startup) configurable. This can help in cases where
the are frequent partitions, or frequent deletion of buckets.
riak_cs_utils:fetch_user was returning the wrong boolean for
`KeepDeleted` buckets. This meant that buckets were being pruned in the
opposite scenario from when they should. This change also prunes buckets
even when there are no siblings.
@shino shino added this to the 1.4.5 milestone Feb 20, 2014
@shino shino added the Bug label Feb 20, 2014
@kuenishi
Copy link
Contributor

  • r_t
  • test / pulse
  • xref
  • dialyzer
  • review

user_buckets_prune_time() ->
get_env(riak_cs, user_buckets_prune_time, ?USER_BUCKETS_PRUNE_TIME).

-spec set_user_buckets_prune_time(integer()) -> ok | {error, invalid_value}.
Copy link
Contributor

Choose a reason for hiding this comment

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

pos_integer() instead of integer() like above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 908f373.

@kuenishi
Copy link
Contributor

Also I would suggest improving spec of fetch_user/1 to:

-spec fetch_user(KeyId::binary(), pid()) ->
                        {ok, {riakc_obj(), ShouldKeepDeletedBuckets::boolean()}} |
                        {error, term()}.

For clearer understanding and preventing misuse of boolean.

@kuenishi
Copy link
Contributor

Looks like this also fixes #799. riak_cs_storage:sum_user/2 also uses get_user.

@kuenishi
Copy link
Contributor

+1 to merge regardless of above nitpick comments. Also confirmed that creating bucket after deleting another bucket removes old deleted bucket.

Eventually we'd add riak_test about this?

-spec cleanup_bucket(cs_bucket()) -> boolean().
cleanup_bucket(?RCS_BUCKET{last_action=created}) ->
false;
cleanup_bucket(?RCS_BUCKET{last_action=deleted,
modification_time=ModTime}) ->
timer:now_diff(os:timestamp(), ModTime) > 86400.
timer:now_diff(os:timestamp(), ModTime) >
riak_cs_config:user_buckets_prune_time().
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for stepping in and I don't execute this fix ...
timer:now_diff/2 returns time interval in micro seconds and riak_cs_config:user_buckets_prune_time() seems to be represented by seconds. Any mismatch??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch! Fixed in 3ffac5d.

This fixes a bug where seconds were used to compare with microseconds.
This would cause deleted buckets to be pruned two orders of magnitude
before they were supposed to.
@reiddraper
Copy link
Contributor Author

  • Docs/specs addressed in 908f373.
  • Microsecond mixup addressed in 3ffac5d.

@reiddraper
Copy link
Contributor Author

I'm re-running all tests now.

@reiddraper
Copy link
Contributor Author

Eventually we'd add riak_test about this?

Yes. I'd like to try and think of a way of testing it that doesn't resort to timer:sleep/1, but maybe that's what we have to do?

@andrewjstone
Copy link
Contributor

code changes look good. dialyzer is clean regarding changes. tests running now.

@andrewjstone
Copy link
Contributor

👍

reiddraper added a commit that referenced this pull request Feb 20, 2014
@reiddraper reiddraper merged commit cb3e606 into release/1.4 Feb 20, 2014
@reiddraper reiddraper deleted the bugfix/prune-user-buckets branch February 20, 2014 19:32
@kuenishi
Copy link
Contributor

+1, yay!

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

Successfully merging this pull request may close these issues.

None yet

4 participants