KAFKA-2120

[MayureshGharat]

Trivial fix to get rid of unused statements in kafkaProducer.

[eribeiro]

@MayureshGharat There's a typo here:

kafka/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java

Line 682 in c67ca65

* Check and may be get the time elapsed since startTime.

it's maybe, not may be ;)

Also, it would be nice to be detailed about the error by informing the value and the threshold that was violated:

kafka/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java

Line 692 in c67ca65

throw new TimeoutException("Request timed out");

In this case, the exception is more of a side effect of this function -- and so, better to be documented in the @throws clause of the comment -- than a proper goal of this method, that is, afaik, to retrieve the remaining time.

kafka/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java

Line 683 in c67ca65

* Throws a {@link org.apache.kafka.common.errors.TimeoutException} if the elapsed time

Finally, this checkMaybeGetRemainingTime is a mouthful of a name and mixes two duties for a single method: it checks if the threshold was violated and retrieves the value otherwise. I would suggest to rename it to checkAndGetRemainingTime or something alike, because it only "maybe" retrieves the time if the check doesn't passes so "maybe" is superfluous here, imho. Up to you.

[guozhangwang]

Since the purpose of this PR is just a follow-up cleanup of KAFKA-2120, I am fine either addressing @eribeiro 's comments in another MINOR PR or in this PR as well if @MayureshGharat would like to do so.

[MayureshGharat]

@guozhangwang I will make the change for @throw and also the threshold message in this PR. I am not sure if we should change the method name. I had discussed this with Joel and we had comeup with that name. Its little verbose but it kind of explains what might happen. Does that sound good ?

[guozhangwang]

I am OK with the current function name too, it is up to you for how to address the comments :)

[MayureshGharat]

@eribeiro
Adding the threshold is a great suggestion. I will upload a new PR for the same :)
Regarding "In this case, the exception is more of a side effect of this function -- and so, better to be documented in the @throws clause of the comment -- than a proper goal of this method, that is, afaik, to retrieve the remaining time.",
The main goal is to first check and return the value only and only if the check is valid.

As per http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html#throwstag, the purpose of the @throws tag is to indicate which exceptions the programmer must catch (for checked exceptions) or might want to catch (for unchecked exceptions). (It is considered poor programming practice to include unchecked exceptions in the throws clause.)
Also TimeoutException is a type of KafkaException, which is an unchecked exception. . Also the checkMaybeGetRemainingTime() is a private method and is only used in the Kafkaproducer.send(). so if we are throwing the TimeoutException from checkMaybeGetRemainingTime(), it will be caught in Kafkaproducer.send() as a KafkaException and rethrown to the user.

[eribeiro]

@MayureshGharat +1 about your comments. :) After posting the comment, I ended up in the http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html page too, so I totally see the point of throws. Agree with you again. I am okay with keeping the name as-is too. No problem. :)

[guozhangwang]

LGTM, thanks @MayureshGharat