Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2380#discussion_r147053713
--- Diff:
external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java
---
@@ -438,55 +440,53 @@ private void commitOffsetsForAckedTuples() {
// ======== Ack =======
@Override
public void ack(Object messageId) {
- if (!isAtLeastOnce()) {
- // Only need to keep track of acked tuples if commits are done
based on acks
- return;
- }
-
+ // Only need to keep track of acked tuples if commits to Kafka are
done after a tuple ack is received
final KafkaSpoutMessageId msgId = (KafkaSpoutMessageId) messageId;
- if (!emitted.contains(msgId)) {
- if (msgId.isEmitted()) {
- LOG.debug("Received ack for message [{}], associated with
tuple emitted for a ConsumerRecord that "
- + "came from a topic-partition that this consumer
group instance is no longer tracking "
- + "due to rebalance/partition reassignment. No action
taken.", msgId);
+ if (isAtLeastOnceProcessing()) {
--- End diff --
@hmcl I agree that
```
if condition == false
do nothing
else
do something
```
is bad, but as @HeartSaVioR mentions early return (guard clause) isn't the
same thing. Instead of writing
```
if precondition == true
rest of function
return
```
it can make the function more readable to write
```
if precondition == false
return
rest of function
```
because the guarding `if` now doesn't cover the rest of the function, and
the indendation level is reduced. See
https://blog.codinghorror.com/flattening-arrow-code/ or
https://en.wikibooks.org/wiki/Computer_Programming/Coding_Style/Minimize_nesting#Early_return.
I agree that since there's no code after the `if` closes it's not that
important. Will leave it up to your judgement.
---
