Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

pubsub: introduce pubsub chan Unsub(), fix few sync tests #2109

Merged
merged 4 commits into from
Nov 23, 2020

Conversation

eduser25
Copy link
Contributor

@eduser25 eduser25 commented Nov 21, 2020

  • Introduces Unsub() to pubsub
  • adds a Unsub() test
  • moves few tests to use pubsub and not read from individual channels

fixes #2108

  • Control Plane [X]
  • Tests [X]

  • Does this change contain code from or inspired by another project? If so, did you notify the maintainers and provide attribution?
    No

@eduser25 eduser25 requested a review from a team as a code owner November 21, 2020 02:35
- Introduces Close() to pubsub
- adds a Close() test
- moves few tests to use pubsub and not read from individual channels

Signed-off-by: Eduard Serra <eduser25@gmail.com>
@eduser25 eduser25 changed the title pubsub: introduce close, fix few sync tests pubsub: introduce pubsub chan Close(), fix few sync tests Nov 21, 2020
pkg/kubernetes/client_test.go Outdated Show resolved Hide resolved
pkg/kubernetes/events/event_pubsub.go Outdated Show resolved Hide resolved
shashankram
shashankram previously approved these changes Nov 23, 2020
- naming: Close() -> Unsub()
- Add release branch references vs main
- use idiomatic channel range to exit on close

Signed-off-by: Eduard Serra <eduser25@gmail.com>
shashankram
shashankram previously approved these changes Nov 23, 2020
@eduser25 eduser25 changed the title pubsub: introduce pubsub chan Close(), fix few sync tests pubsub: introduce pubsub chan Unsub(), fix few sync tests Nov 23, 2020
@eduser25 eduser25 requested a review from a team November 23, 2020 18:43
Copy link
Contributor

@draychev draychev left a comment

Choose a reason for hiding this comment

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

What's the use case for Unsub?
Would it make sense to spell out the entire word?

pkg/kubernetes/events/event_pubsub.go Outdated Show resolved Hide resolved
// Unsub is the Unsub implementation for PubSub.
// It is synchronized, upon exit the channel is guaranteed to be both
// unsubbed to all topics and closed.
// This is a necessary step to guarantee garbage collection
Copy link
Contributor

Choose a reason for hiding this comment

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

At what point in the lifecycle of the controller will this be used? I see that this is needed for garbage collection, but when will it be used, by who, and why?

Copy link
Contributor Author

@eduser25 eduser25 Nov 23, 2020

Choose a reason for hiding this comment

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

At what point in the lifecycle of the controller will this be used?

None currently. It's a good observation though, none of the pubsub channels are currently freed or released during the lifetime of the controller.

when will it be used, by who, and why?

This will be immediately used by unit tests. If we don't release the channels on the pubsub backend, they are still held and subscribed to the relevant topics accross different unit tests contexts, even when closed from the reader side.

Most of all, I think it's a relevant API to provide. There might be instances/parts of the code that we might have the need to release a certain pubsub resource, and we must be able to do it properly without leaking or leaving stuff on the pubsub backend.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation! Might be good to drop a comment for clarity. LGTM

Co-authored-by: Delyan Raychev <delyan.raychev@microsoft.com>
Signed-off-by: Eduard Serra <eduser25@gmail.com>
@codecov-io
Copy link

Codecov Report

Merging #2109 (9c1c02f) into main (306dac8) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2109      +/-   ##
==========================================
+ Coverage   58.49%   58.51%   +0.01%     
==========================================
  Files         144      144              
  Lines        5910     5915       +5     
==========================================
+ Hits         3457     3461       +4     
- Misses       2450     2451       +1     
  Partials        3        3              
Impacted Files Coverage Δ
pkg/kubernetes/events/event_pubsub.go 100.00% <100.00%> (ø)
pkg/envoy/route/config.go 95.23% <0.00%> (-0.80%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 306dac8...9c1c02f. Read the comment docs.

@eduser25 eduser25 merged commit 14be75a into openservicemesh:main Nov 23, 2020
@eduser25 eduser25 deleted the fixes-pubsub branch November 23, 2020 20:28
draychev pushed a commit to draychev/osm that referenced this pull request Dec 14, 2020
…emesh#2109)

* pubsub: introduce close, fix few sync tests

- Introduces Close() to pubsub
- adds a Close() test
- moves few tests to use pubsub and not read from individual channels

* CR

- naming: Close() -> Unsub()
- Add release branch references vs main
- use idiomatic channel range to exit on close

Signed-off-by: Eduard Serra <eduser25@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

flaky test in pkg/smi resulting in panic
4 participants