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

rados: implement watch/notify APIs #634

Merged
merged 2 commits into from
Feb 10, 2022
Merged

rados: implement watch/notify APIs #634

merged 2 commits into from
Feb 10, 2022

Conversation

ansiwen
Copy link
Collaborator

@ansiwen ansiwen commented Jan 31, 2022

This change implements the bindings for the watch/notify APIs of
librados. Instead of callbacks, the watcher is implemented with more
Go-idiomatic channels. A watcher object exposes two read-only
channels, one to receive the notify events and one to receive
occuring errors.

Signed-off-by: Sven Anderson sven@redhat.com

Checklist

  • Added tests for features and functional changes
  • Public functions and types are documented
  • Standard formatting is applied to Go code
  • Is this a new API? Is this new API marked PREVIEW?

@ansiwen ansiwen force-pushed the pr/ansiwen/watcher branch 3 times, most recently from 9364784 to 90f27f7 Compare January 31, 2022 12:07
@ansiwen ansiwen marked this pull request as ready for review January 31, 2022 13:34
@ansiwen ansiwen added the API This PR includes a change to the public API of a go-ceph package label Jan 31, 2022
rados/watcher.go Outdated Show resolved Hide resolved
@phlogistonjohn
Copy link
Collaborator

I've given it a quick skim and it generally looks good so far. I'm sure I'll have some more comments when I look deeper but I think it'll be really good to have this.

Copy link
Collaborator

@phlogistonjohn phlogistonjohn left a comment

Choose a reason for hiding this comment

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

As before I think the general structure looks good and the test appear pretty thorough, thanks! There are a few things here and there that I think we should clean up, but I don't think they're invasive changes.

rados/watcher.go Outdated Show resolved Hide resolved
rados/watcher_test.go Show resolved Hide resolved
rados/watcher.go Outdated Show resolved Hide resolved
@mergify
Copy link

mergify bot commented Feb 7, 2022

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

@ansiwen ansiwen force-pushed the pr/ansiwen/watcher branch 2 times, most recently from 9ad592e to 732161e Compare February 8, 2022 01:09
@mergify
Copy link

mergify bot commented Feb 8, 2022

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

rados/watcher.go Show resolved Hide resolved
rados/watcher.go Show resolved Hide resolved
rados/watcher.go Outdated Show resolved Hide resolved
@ansiwen ansiwen requested a review from nixpanic February 9, 2022 13:38
nixpanic
nixpanic previously approved these changes Feb 9, 2022
@nixpanic
Copy link
Member

nixpanic commented Feb 9, 2022

@ansiwen you'll need to manually resolve the merge conflicts, unfortortunately

Copy link
Collaborator

@phlogistonjohn phlogistonjohn left a comment

Choose a reason for hiding this comment

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

I would like to see the parsing code simplified a bit. That's all that remains.

This change implements the bindings for the watch/notify APIs of
librados. Instead of callbacks, the watcher is implemented with more
Go-idiomatic channels. A watcher object exposes two read-only
channels, one to receive the notify events and one to receive
occuring errors.

Signed-off-by: Sven Anderson <sven@redhat.com>
Signed-off-by: Sven Anderson <sven@redhat.com>
Copy link
Collaborator

@phlogistonjohn phlogistonjohn left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mergify mergify bot merged commit d3a4a05 into master Feb 10, 2022
@mergify mergify bot deleted the pr/ansiwen/watcher branch February 10, 2022 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API This PR includes a change to the public API of a go-ceph package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants