MINOR: improve JavaDocs for consumer CloseOptions by mjsax · Pull Request #19546 · apache/kafka

mjsax · 2025-04-24T02:39:54Z

Reviewers: TengYao Chi frankvicky@apache.org, PoAn Yang
payang@apache.org, Lianet Magrans lmagrans@confluent.io, Anna
Sophie Blee-Goldman ableegoldman@apache.org

frankvicky

@mjsax: Thanks for the significant improvement to the documentation!
Overall LGTM.
I have just two nits for consideration.

frankvicky · 2025-04-24T07:10:01Z

-     * If no thread is blocking in a method which can throw {@link org.apache.kafka.common.errors.WakeupException}, the next call to such a method will raise it instead.
+     * Close the consumer cleanly. {@link CloseOptions} allows to specify a timeout and a
+     * {@link CloseOptions.GroupMembershipOperation membership operation}.
+     * If no timeout is specified, the default timeout of 30 seconds is uses.


Suggested change

* If no timeout is specified, the default timeout of 30 seconds is uses.

* If no timeout is specified, the default timeout of 30 seconds is used.

frankvicky · 2025-04-24T07:11:11Z

+     * This method waits up to the timeout for the consumer to complete pending commits and may leave the group,
+     * depending on the specified membership operation.
+     * If auto-commit is enabled, this will commit the current offsets if possible within the
+     * timeout. If the consumer is unable to complete offset commits and gracefully leave the group (if applicable)


Suggested change

* timeout. If the consumer is unable to complete offset commits and gracefully leave the group (if applicable)

* timeout. If the consumer is unable to complete offset commits and gracefully leaves the group (if applicable)

yeah, a bit confusing, but in this case "leave" is correct, referring to "unable to ...leave"

FrankYang0529 · 2025-04-24T07:40:55Z

     * @throws InterruptException If the thread is interrupted before or while this function is called
     * @throws org.apache.kafka.common.KafkaException for any other error during close
     */
+    @Deprecated


Suggested change

@Deprecated

@Deprecated(since = "4.0")

since 4.1 really (helpful adding the since btw)

Well, 4.1 :) -- c&p error form the previous PR...

My bad 😅

FrankYang0529 · 2025-04-24T08:01:02Z

+     * Note that the execution time of callbacks (such as {@link OffsetCommitCallback} and
+     * {@link ConsumerRebalanceListener}) does not consume time from the close timeout.
+     *
+     * @param option see {@link CloseOptions}; cannot be {@code null}


We don't check whether CloseOptions is null. If users use close((CloseOptions) null), they get NullPointerException. Do we want to return IllegalArgumentException when input is null here? Thanks.

yeah lets add a check and throw an IAException

I think it's ok if users get NPE -- we say "cannot be null" so NPE should be expected?

yeah, I personally prefer IAE too, but we do have this same NPE behaviour on args in many other consumer APIs too (ex. commit, position, committed,...I think almost all)

fair enough

Ok, so the verdict is we keep NPE. Thanks.

(Btw: also aligns to what KS does.)

lianetm

Thanks @mjsax !

lianetm · 2025-04-24T13:24:58Z

+     * <p>
+     * Close the consumer with {@link CloseOptions.GroupMembershipOperation#DEFAULT default membership operation}
+     * cleanly within the specified timeout. This method waits up to
+     * {@code timeout} for the consumer to complete pending commits and may leave the group (if applicable).


about the "and may leave the group (if applicable).", could we be explicit in this case, just to make things clear? (it will leave the group if it's a dynamic member). That's the only case here that the close has the DEFAULT membership behaviour I expect.

agreed, "if applicable" is pretty vague and I'd be left wondering if I saw this as a user

lianetm · 2025-04-24T13:28:13Z

-     * {@code timeout} for the consumer to complete pending commits and leave the group.
+     * This method has been deprecated since Kafka 4.0 and should use {@link KafkaConsumer#close(CloseOptions)} instead.
+     * <p>
+     * Close the consumer with {@link CloseOptions.GroupMembershipOperation#DEFAULT default membership operation}


"membership operation" seems to generic to me really, this is only about "leave group behaviour" or similar. Would that be clearer as java doc?

lianetm · 2025-04-24T13:29:16Z

+     * Close the consumer cleanly. {@link CloseOptions} allows to specify a timeout and a
+     * {@link CloseOptions.GroupMembershipOperation membership operation}.
+     * If no timeout is specified, the default timeout of 30 seconds is uses.
+     * If no membership operation is specified, the {@link CloseOptions.GroupMembershipOperation#DEFAULT default


as above, would ~"leave group behaviour" be clearer than the generic "membership operation"?

agreed, would also be good to specify what the default leave behavior actually is

lianetm · 2025-04-24T13:29:31Z

+     * If no membership operation is specified, the {@link CloseOptions.GroupMembershipOperation#DEFAULT default
+     * membership operation} is used.
+     * <p>
+     * This method waits up to the timeout for the consumer to complete pending commits and may leave the group,


Suggested change

* This method waits up to the timeout for the consumer to complete pending commits and may leave the group,

* This method waits up to the timeout for the consumer to complete pending commits and maybe leave the group,

lianetm · 2025-04-24T13:45:27Z

     * @throws InterruptException If the thread is interrupted before or while this function is called
     * @throws org.apache.kafka.common.KafkaException for any other error during close
     */
+    @Deprecated


since 4.1 really (helpful adding the since btw)

lianetm · 2025-04-24T13:48:17Z

+     * This method waits up to the timeout for the consumer to complete pending commits and may leave the group,
+     * depending on the specified membership operation.
+     * If auto-commit is enabled, this will commit the current offsets if possible within the
+     * timeout. If the consumer is unable to complete offset commits and gracefully leave the group (if applicable)


yeah, a bit confusing, but in this case "leave" is correct, referring to "unable to ...leave"

ableegoldman · 2025-04-24T23:39:20Z

    /**
-     * Tries to close the consumer cleanly within the specified timeout. This method waits up to
-     * {@code timeout} for the consumer to complete pending commits and leave the group.
+     * This method has been deprecated since Kafka 4.0 and should use {@link KafkaConsumer#close(CloseOptions)} instead.


Suggested change

* This method has been deprecated since Kafka 4.0 and should use {@link KafkaConsumer#close(CloseOptions)} instead.

* This method has been deprecated since Kafka 4.1 and should use {@link KafkaConsumer#close(CloseOptions)} instead.

ableegoldman · 2025-04-24T23:40:56Z

+     * {@code timeout} for the consumer to complete pending commits and may leave the group (if applicable).
     * If auto-commit is enabled, this will commit the current offsets if possible within the
-     * timeout. If the consumer is unable to complete offset commits and gracefully leave the group
+     * timeout. If the consumer is unable to complete offset commits and gracefully leave the group (if applicable)


(if applicable) is ok to use here as long as we specified when it's applicable earlier in the javadocs

mjsax · 2025-04-25T17:14:18Z

Updates this PR. Call for review.

lianetm

Thanks for the updates! Minor comments left, and also, could we fix this log that references the deprecated close?

kafka/clients/src/main/java/org/apache/kafka/clients/consumer/internals/Fetcher.java

Line 141 in 6462f7a

"KafkaConsumer.close(Duration timeout)", timer.timeoutMs());

I would personally remove the explicit func name from the log and just mention "the close operation/call", but as you prefer.

lianetm · 2025-04-25T18:27:20Z

+     * <p>
+     * Close the consumer with {@link CloseOptions.GroupMembershipOperation#DEFAULT default leave group behavior}
+     * cleanly within the specified timeout. This method waits up to
+     * {@code timeout} for the consumer to complete pending commits and maybe leaves the group (if the group is dynamic).


Suggested change

* {@code timeout} for the consumer to complete pending commits and maybe leaves the group (if the group is dynamic).

* {@code timeout} for the consumer to complete pending commits and maybe leave the group (if the member is dynamic).

lianetm · 2025-04-25T18:28:59Z

+     * If no leave group behavior is specified, the {@link CloseOptions.GroupMembershipOperation#DEFAULT default
+     * leave group behavior} is used.
+     * <p>
+     * This method waits up to the timeout for the consumer to complete pending commits and maybe leaves the group,


Suggested change

* This method waits up to the timeout for the consumer to complete pending commits and maybe leaves the group,

* This method waits up to the timeout for the consumer to complete pending commits and maybe leave the group,

Cannot follow.

This method wait[s] ... and maybe leave[s] the group. -- Why would we remove the s?

she's right, no s. English is confusing

i don't know the proper grammar terminology here but essentially it's because the "maybe leave the group" is part of the X in the sentence "this method waits up to the timeout for the consumer to X"

here the "maybe leave the group" comes after an "and" which can be amiguous but in this context I think it makes more sense for it to be part of the X, ie "this method waits up to the timeout for the consumer to (complete pending commits and maybe leave the group)"

so its should be conjugated the same way as the thing before the and, ie the "complete pending commits"

since it's "complete" and not "completes", it should also be "leave" and not "leaves"

technically the original sentence is also grammatically correct, however it doesn't mean exactly the same thing.

With the s it's is effectively saying "This method (waits up to the timeout for the consumer to complete pending commits) and maybe leaves the group" which implies leaving the group isn't part of the timeout.

Without the s it's grouped as "this method waits up to the timeout for the consumer to (complete pending commits and maybe leave the group)" which implies leaving the group is part of the timeout

To make matters even more confusing, I noticed we actually do not pass the timeout into the consumer or producer's #close methods...

So technically-technically the original sentence is also correct code-wise, but imo that's kind of a bug

Thanks for explaining. "Surprisingly", I could follow and it make sense to me 😂 -- Mr.John (english major) @vvcephei could not have explained it any better.

lianetm · 2025-04-25T18:29:38Z

+     * This method waits up to the timeout for the consumer to complete pending commits and maybe leaves the group,
+     * depending on the specified leave group behavior.
+     * If auto-commit is enabled, this will commit the current offsets if possible within the
+     * timeout. If the consumer is unable to complete offset commits and gracefully leaves the group (if applicable)


Suggested change

* timeout. If the consumer is unable to complete offset commits and gracefully leaves the group (if applicable)

* timeout. If the consumer is unable to complete offset commits and gracefully leave the group (if applicable)

lianetm

Thanks @mjsax ! LGTM.

Reviewers: TengYao Chi <frankvicky@apache.org>, PoAn Yang <payang@apache.org>, Lianet Magrans <lmagrans@confluent.io>, Anna Sophie Blee-Goldman <ableegoldman@apache.org>

MINOR: improve JavaDocs for consumer CloseOptions

514ad0e

mjsax added consumer clients labels Apr 24, 2025

github-actions Bot added the small Small PRs label Apr 24, 2025

mjsax mentioned this pull request Apr 24, 2025

KAFKA-16758: Extend Consumer#close with an option to leave the group or not #17614

Merged

follow up

ec132e5

frankvicky reviewed Apr 24, 2025

View reviewed changes

FrankYang0529 reviewed Apr 24, 2025

View reviewed changes

lianetm reviewed Apr 24, 2025

View reviewed changes

ableegoldman reviewed Apr 24, 2025

View reviewed changes

review comments

e0056ea

lianetm reviewed Apr 25, 2025

View reviewed changes

mjsax added 3 commits April 25, 2025 16:37

review

6b23aeb

fetcher log

61aa266

Writing proper English.

5727ffb

lianetm approved these changes Apr 29, 2025

View reviewed changes

mjsax merged commit 3bb15c5 into apache:trunk Apr 29, 2025
22 checks passed

mjsax deleted the minor-close-option branch April 29, 2025 23:38

	* If no timeout is specified, the default timeout of 30 seconds is uses.
	* If no timeout is specified, the default timeout of 30 seconds is used.

	* timeout. If the consumer is unable to complete offset commits and gracefully leave the group (if applicable)
	* timeout. If the consumer is unable to complete offset commits and gracefully leaves the group (if applicable)

	* This method waits up to the timeout for the consumer to complete pending commits and may leave the group,
	* This method waits up to the timeout for the consumer to complete pending commits and maybe leave the group,

	* This method has been deprecated since Kafka 4.0 and should use {@link KafkaConsumer#close(CloseOptions)} instead.
	* This method has been deprecated since Kafka 4.1 and should use {@link KafkaConsumer#close(CloseOptions)} instead.

	* {@code timeout} for the consumer to complete pending commits and maybe leaves the group (if the group is dynamic).
	* {@code timeout} for the consumer to complete pending commits and maybe leave the group (if the member is dynamic).

Conversation

mjsax commented Apr 24, 2025 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frankvicky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lianetm Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lianetm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjsax commented Apr 25, 2025

Uh oh!

lianetm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mjsax commented Apr 24, 2025 •

edited by github-actions Bot

Loading

lianetm Apr 25, 2025 •

edited

Loading