move register callback to non builder inference #94
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #94 +/- ##
============================================
- Coverage 75.40% 75.25% -0.15%
- Complexity 238 240 +2
============================================
Files 44 44
Lines 1061 1063 +2
Branches 88 87 -1
============================================
Hits 800 800
- Misses 226 228 +2
Partials 35 35
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Test Results15 files ±0 15 suites ±0 30s ⏱️ -1s Results for commit 17f50bf. ± Comparison against base commit df4f796. This pull request removes 5 and adds 5 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
| @@ -30,20 +30,29 @@ | |||
| * for sample usage and test. Note that testing requires Thread.sleep > poll timeout in between | |||
| */ | |||
| public class KafkaLiveEventListener<K, V> implements AutoCloseable { | |||
| Queue<BiConsumer<? super K, ? super V>> callbacks; | |||
There was a problem hiding this comment.
can this be private final? Also suggest making it a Collection - the queue is an impl detail that could just as easily be satisfied with a different collection implementation.
| @@ -54,13 +63,15 @@ public void close() throws Exception { | |||
| } | |||
|
|
|||
| public static final class Builder<K, V> { | |||
| List<BiConsumer<? super K, ? super V>> callbacks = new ArrayList<>(); | |||
| Queue<BiConsumer<? super K, ? super V>> deprecatedCallbacksFlow = new ConcurrentLinkedQueue<>(); | |||
There was a problem hiding this comment.
nit - looks like these were these exposed to package before? Since we're technically breaking it here anyway, does it need to be?
naming - this isn't really a flow, it's used by the flow. I'd just leave it as callbacks and mark it deprecated
| @@ -73,17 +84,17 @@ public Builder<K, V> withExecutorService( | |||
|
|
|||
| public KafkaLiveEventListener<K, V> build( | |||
| String consumerName, Config kafkaConfig, Consumer<K, V> kafkaConsumer) { | |||
| assertCallbacksPresent(); | |||
| Queue<BiConsumer<? super K, ? super V>> callbacks; | |||
| if (deprecatedCallbacksFlow.size() > 0) { | |||
There was a problem hiding this comment.
Do we need these size check here? if nothing was added to it, then it's already an empty queue.
| ExecutorService executorService, | ||
| boolean cleanupExecutor) { | ||
| this.callbacks = callbacks; |
There was a problem hiding this comment.
As we had discussed the other day either here or in the builder (ideally here), we should copy off the array.
The usage of a concurrent queue is an impl detail of this class only, so shouldn't rely on a concurrent impl being passed in - the builder didn't even need to change since it doesn't have its own requirement for concurrency.
| ExecutorService executorService = Executors.newSingleThreadExecutor(); | ||
| boolean cleanupExecutor = | ||
| private final Collection<BiConsumer<? super K, ? super V>> callbacks = | ||
| new ConcurrentLinkedQueue<>(); |
There was a problem hiding this comment.
nit - no reason to use a concurrent queue here. No concurrency requirements in the builder and this will be copied into a concurrent queue when the listener is built.
There was a problem hiding this comment.
yeah I realised it, felt no harm to have a concurrent safe data structure because in deprecated method we suggested to have singleton instance of builder
|
Approved. I would like to revisit this listener framework and usage once. |
No description provided.