Github user hmcl commented on a diff in the pull request:

    https://github.com/apache/storm/pull/1679#discussion_r79794331
  
    --- Diff: 
external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java
 ---
    @@ -479,16 +482,17 @@ public OffsetAndMetadata findNextCommitOffset() {
                 KafkaSpoutMessageId nextCommitMsg = null;     // this is a 
convenience variable to make it faster to create OffsetAndMetadata
     
                 for (KafkaSpoutMessageId currAckedMsg : ackedMsgs) {  // 
complexity is that of a linear scan on a TreeMap
    -                if ((currOffset = currAckedMsg.offset()) == 
initialFetchOffset || currOffset == nextCommitOffset + 1) {            // found 
the next offset to commit
    +                if ((currOffset = currAckedMsg.offset()) == 
nextCommitOffset + 1) {            // found the next offset to commit
    --- End diff --
    
    @srdo from what I recall, in scenarios where topic had no messages at all, 
this was necessary. I tuned some of these changes based on several integration 
tests I ran under several scenarios. As I mentioned in a prior comment, I have 
been in touch with people that are running this in production without any 
issues. You are proposing quite a few algorithmic changes. Are these changes 
caused by bugs ? Do you have test scenarios that guarantee that the proposed 
changes are not adding new bugs ? Or are these changes proposed by code 
analysis that leads you to believe that they are not necessary ?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Reply via email to