KAFKA-20341: Clarify return semantics of assignment(), subscription(), and metrics() in consumer Javadoc#21849
KAFKA-20341: Clarify return semantics of assignment(), subscription(), and metrics() in consumer Javadoc#21849DL1231 wants to merge 1 commit intoapache:trunkfrom
Conversation
github-actions
bot
added
triage
| acquireAndEnsureOpen(); | ||
| try { | ||
| return Collections.unmodifiableSet(subscriptions.assignedPartitions()); | ||
| return Set.copyOf(subscriptions.assignedPartitions()); |
There was a problem hiding this comment.
The subscriptions.assignedPartitions() already returns a new collection. Making another one won't make memory walk the plank, but it is definitely unnecessary, right?
There was a problem hiding this comment.
same goes to ClassicKafkaConsumer too.
| * <p>The returned map is an unmodifiable live view of the metrics. Changes to the underlying | ||
| * metrics will be reflected in the returned map. | ||
| * | ||
| * @return An unmodifiable live view of the map of metrics currently maintained by the consumer |
There was a problem hiding this comment.
For the sake of consistency, could we apply these excellent javadocs to Admin and Producer too?
| @@ -1400,6 +1408,11 @@ public Uuid clientInstanceId(Duration timeout) { | |||
|
|
|||
| /** | |||
| * Get the metrics kept by the consumer | |||
There was a problem hiding this comment.
nit:indentation issue
| * <p>The returned map is an unmodifiable live view of the metrics. Changes to the underlying | ||
| * metrics will be reflected in the returned map. | ||
| * | ||
| * @return An unmodifiable live view of the map of metrics currently maintained by the consumer |
There was a problem hiding this comment.
ShareConsumerImpl.metrics() uses Map.copyOf(). unmodifiable live view looks incorrect here.
There was a problem hiding this comment.
we've reverted the change to use Collections.unmodifiableMap. see #21832
| acquireAndEnsureOpen(); | ||
| try { | ||
| return Collections.unmodifiableSet(this.subscriptions.assignedPartitions()); | ||
| return Set.copyOf(this.subscriptions.assignedPartitions()); |
There was a problem hiding this comment.
assignment() and subscription() should both follow the same snapshot rule. any reason for not changing subscription() here.
3 participants
Improves Javadoc to clarify the return semantics of three methods:
assignment()andsubscription(): return an immutablesnapshot at the time of the call. -
metrics(): returns anunmodifiable live view. The Javadoc now documents this clearly;
the implementation is unchanged from the original behavior.