MINOR: Share group tidying#21711
Conversation
|
I'll do a follow-on PR to tidy up |
Minor tidying up of share group code, mainly fixing compiler warnings. Also fixed three flaky tests, all of which were failing occasionally because of impatience. Reviewers: Manikumar Reddy <manikumar.reddy@gmail.com>
chia7712
left a comment
chia7712
left a comment
There was a problem hiding this comment.
@AndrewJSchofield @omkreddy Sorry for the late noise! Left some minor comments for better decoration 😄
| Config config = adminClient.describeConfigs(List.of(groupConfigResource)).all().get().get(groupConfigResource); | ||
| ConfigEntry entry = config.get(GroupConfig.SHARE_DELIVERY_COUNT_LIMIT_CONFIG); | ||
| return entry != null && entry.value().equals(newValue); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
nit: We don't need to catch the exception here, as TestUtils.waitForCondition already handle it
| private void alterShareDeliveryCountLimit(String groupId, String newValue) { | ||
| alterShareGroupConfig(groupId, GroupConfig.SHARE_DELIVERY_COUNT_LIMIT_CONFIG, newValue); | ||
|
|
||
| // This config is changed dynamically in tests, and we need it to have propagated before the test proceeds. |
There was a problem hiding this comment.
Should we add this wait to alterShareGroupConfig so that all dynamic updates can benefit from it?
There was a problem hiding this comment.
Yes, I have this on my to-do list.
| acquireAndEnsureOpen(); | ||
| try { | ||
| return Collections.unmodifiableSet(subscriptions.subscription()); | ||
| return Set.copyOf(subscriptions.subscription()); |
There was a problem hiding this comment.
Should we apply this change to AsyncKafkaConsumer#subscription for the sake of consistency?
| @Override | ||
| public Map<MetricName, ? extends Metric> metrics() { | ||
| return Collections.unmodifiableMap(metrics.metrics()); | ||
| return Map.copyOf(metrics.metrics()); |
There was a problem hiding this comment.
unlike subscription, which is always a snapshot of the current state, metrics.metrics() returns the underlying collection. This change would shift the returned metrics from a live view to a static snapshot. While this behavior is currently undocumented, it would be good to align it with other client components for the consistency
Resolves a couple of review comments from #21711. The remaining comments are in `ShareConsumerTest` and will be addressed separately. Reviewers: Lianet Magrans <lmagrans@confluent.io>, Viktor Somogyi-Vass <viktorsomogyi@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>
Resolves a couple of review comments from #21711. The remaining comments are in `ShareConsumerTest` and will be addressed separately. Reviewers: Lianet Magrans <lmagrans@confluent.io>, Viktor Somogyi-Vass <viktorsomogyi@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>
3 participants
Minor tidying up of share group code, mainly fixing compiler warnings.
Also fixed three flaky tests, all of which were failing occasionally
because of impatience.
Reviewers: Manikumar Reddy manikumar.reddy@gmail.com