MINOR: Rename ambiguous method name

[hgh1472]

While reading through the code, I found the method name to be somewhat
ambiguous and not fully descriptive of its purpose.

So I renamed the method to make its purpose clearer and more
self-explanatory. If there was another reason for the original naming,
I’d be happy to hear about it.

Reviewers: Lianet Magrans lmagrans@confluent.io

[lianetm]

Hi @hgh1472 , thanks for the patch! I guess the naming came from the fact that this throws only under a certain condition (if the group id is empty), therefore "maybeThrow...", makes sense?

That being said, we can surely improve readability like you're suggesting, but being explicit about the condition I would say (otherwise I don't see much gain between maybeThrow vs throwIf). So something like throwIfGroupIdEmpty maybe?

Also, we should change consistently on the AsyncKafkaConsumer I would expect. Thanks!

[hgh1472]

@lianetm Thanks for the reply!
I completely agree with your points. My intention was to clearly reflect the specific condition you mentioned.
If you agree with renaming the method, would it be okay for me to proceed with updating AsyncKafkaConsumer accordingly, as mentioned?

[hgh1472]

@lianetm
I've completed the changes as suggested!
I think throwIfGroupIdEmpty is the most appropriate name.

[lianetm]

Thanks for the update! Just a nit, throwIfGroupIdNotDefined or similar is probably the accurate one. The validation for empty group ID happens first, at a higher level in the constructor

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

Line 161 in f866594

throw new InvalidGroupIdException("The configured " + ConsumerConfig.GROUP_ID_CONFIG

, and then we have some APIs that require group Id (so they throwIfGroupIdNotDefined)
(Sorry I suggested the empty myself initially, mixing empty optional vs empty string)

[hgh1472]

@lianetm
Would it be correct to understand your point as saying that throwIfGroupIdEmpty could be confusing because the term “empty” overlaps with both Optional.empty() and an empty String?
In other words, the method is essentially checking whether the groupId is defined or not — is that right?

[lianetm]

the method is essentially checking whether the groupId is defined or not — is that right?

exactly, it's checking that it's defined because those APIs require a groupId (and if it's defined we know already it will be non-empty because that's enforced in the consumer constructor)

[hgh1472]

@lianetm
thanks for catching a detail I could have missed!
I updated the method name accordingly to reflect your feedback.

[lianetm]

Thanks for the improvement @hgh1472! LGTM