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

Add support for conditional postcommit hooks #771

Merged
merged 1 commit into from
Dec 19, 2013

Conversation

jtuple
Copy link
Contributor

@jtuple jtuple commented Dec 19, 2013

This commit adds support for conditional postcommit hooks. Currently, these
are implemented solely for typed buckets, but may be expanded to untyped
buckets in the future.

This addition is motivated by the lack of support for "bucket fixups" for
typed buckets. For traditional buckets, fixups are the primary mechanism
used in Riak to ensure hooks are installed on buckets as necessary.

Unlike traditional postcommits hooks, that are registered per-bucket,
conditional hooks are registered globally (per node). Each time a PUT is
triggered, all registered conditional postcommit hooks are triggered. The
hook function is passed the relevant bucket, key, and bucket properties and
should return either false or a list of postcommit hooks that should be
triggered. If any hooks are returned, they are invoked the same as normal
postcommit hooks.

Test pull-request: basho/riak_test#485

This commit adds support for conditional postcommit hooks. Currently, these
are implemented solely for typed buckets, but may be expanded to untyped
buckets in the future.

This addition is motivated by the lack of support for "bucket fixups" for
typed buckets. For traditional buckets, fixups are the primary mechanism
used in Riak to ensure hooks are installed on buckets as necessary.

Unlike traditional postcommits hooks, that are registered per-bucket,
conditional hooks are registered globally (per node). Each time a PUT is
triggered, all registered conditional postcommit hooks are triggered. The
hook function is passed the relevant bucket, key, and bucket properties and
should return either false or a list of postcommit hooks that should be
triggered. If any hooks are returned, they are invoked the same as normal
postcommit hooks.
Hooks = get_hooks(conditional_postcommit),
ActiveHooks =
[ActualHook || {Mod, Fun} <- Hooks,
ActualHook <- [Mod:Fun(BucketType, Bucket, BucketProps)],
Copy link
Contributor

Choose a reason for hiding this comment

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

The other functions that take bucket type and bucket, including get_conditional_postcommit, typically pass them as a 2-tuple. If we were to extend get_conditional_postcommit to untyped buckets I think we'd probably want to make this a 2-arity function.

@jrwest
Copy link
Contributor

jrwest commented Dec 19, 2013

changes look good. I like the public ets table approach used here w/ the backup to appenv. I left one comment but I don't see that as necessary to change before merge. Test in basho/riak_test#485 was used to verify the changes. In addition, I hacked up the test a bit to ensure that we only run the postcommit hook once if it is in both bucket props and in the conditional hooks as well as a few other cases.

+1 merge.

@jtuple
Copy link
Contributor Author

jtuple commented Dec 19, 2013

Yeah, probably should change things to a 2-tuple in that one case, but since the relevant riak_repl PR that adds Riak's first ever conditional hook is coded to the current approach, I'm going to leave things as-is since we're trying to wrap up things for code freeze.

If/when we get around to extending to untyped buckets, we'll make the change and update existing conditional hooks as necessary.

jtuple added a commit that referenced this pull request Dec 19, 2013
Add support for conditional postcommit hooks
@jtuple jtuple merged commit bb184f3 into develop Dec 19, 2013
@seancribbs seancribbs deleted the jdb-conditional-postcommit branch April 1, 2015 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants