[chore] clean up kafkatopicsobserver #38412
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
axw
force-pushed
the
kafkatopicsobserver-test-speedup
branch
from
c1bb934 to
cc4545c
Compare
Clean up the implementation and speed up the tests from 16s to subsecond. ListEndpoints is meant to be run in a loop by the EndpointsWatcher.ListAndWatchloop. I have refactored the implementation to use the "topics_sync_interval" config as the interval for the watcher, and moved the topic listing/matching directly inside ListEndpoints. Start is now a no-op, like in hostobserver. With this change we can speed up the tests by removing the sleeps, reducing the refresh interval, and using a channel to wait for observations.
axw
force-pushed
the
kafkatopicsobserver-test-speedup
branch
from
cc4545c to
9041271
Compare
|
I don't know why |
| @@ -38,7 +38,7 @@ func TestLoadConfig(t *testing.T) { | |||
| ProtocolVersion: "3.7.0", | |||
| Brokers: []string{"1.2.3.4:9092", "2.3.4.5:9092"}, | |||
| TopicRegex: "^topic[0-9]$", | |||
| TopicsSyncInterval: 5 * time.Second, | |||
There was a problem hiding this comment.
Any particular reason to make this lower?
There was a problem hiding this comment.
To speed up TestCollectEndpointsAllConfigSettings, which currently takes 16 seconds to run, and is now ~instantaneous.
Comment on lines
+75
to
+81
| type kafkaTopicsEndpointsLister struct { | ||
| o *kafkaTopicsObserver | ||
| topicRegexp *regexp.Regexp | ||
|
|
||
| mu sync.Mutex | ||
| topics []string | ||
| } |
There was a problem hiding this comment.
Any reason to move this to own type instead of the original combined?
There was a problem hiding this comment.
Basically the same answer as #38412 (comment), just API hygiene -- the EndpointsLister does not need to be exposed to external users of the observer.
songy23
pushed a commit
that referenced
this pull request
Mar 7, 2025
…ackage (#38416) #### Description Move EndpointsLister and EndpointsWatcher to a new "endpointswatcher" package. The intention here is to more clearly delineates the public API of the observer package -- the bits inside endpointswatcher are intended only for implementations of observers. See #38412 (comment) and #38412 (comment) for further motivation. I've made the new package non-internal in case there are any observer implementations out of tree. #### Link to tracking issue N/A #### Testing Ran/updated unit tests #### Documentation N/A except for adding package doc explaining that endpointswatcher is intended for implementations only.
MovieStoreGuy
approved these changes
Mar 11, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Clean up the implementation and speed up the tests from 16s to subsecond.
ListEndpoints is run in a loop by the EndpointsWatcher.ListAndWatchloop, with differences being reported by a notifier interface - whereas the tests are calling ListEndpoints directly. I have refactored the implementation to use the "topics_sync_interval" config as the interval for the watcher, and moved the topic listing/matching directly inside ListEndpoints. Start is now a no-op, like in hostobserver.
With this change we can speed up the tests by removing the sleeps, reducing the refresh interval, and using a channel to wait for observations.
Link to tracking issue
None
Testing
Run the unit tests.
Before:
After:
Documentation
N/A