STORM-2625: reduce uncommitted count when kafka consumer group re-ass…

[WolfeeTJ]

STORM-2625: reduce uncommitted count when kafka consumer group re-assign partitions

[WolfeeTJ]

@srdo please help to review. I tested locally it should be working. However during Travis CI build another irrelevant test case failed, I think we could safely ignore it. Please check. Thanks.

[ERROR] /home/travis/build/apache/storm/external/storm-kafka/src/test/org/apache/storm/kafka/PartitionManagerTest.java:[81,23] no suitable constructor found for ZkCoordinator(org.apache.storm.kafka.DynamicPartitionConnections,java.util.Map<java.lang.String,java.lang.Object>,org.apache.storm.kafka.SpoutConfig,org.apache.storm.kafka.ZkState,int,int,int,java.lang.String)

[HeartSaVioR]

@WolfeeTJ
Please craft a pull request against master branch unless the patch is only valid for specific branch(es).

[HeartSaVioR]

Forgot to mention: Thanks for the contribution. :)

[srdo]

The build error is unrelated to this change, I think the cherry pick of https://github.com/apache/storm/pull/2195/files to 1.1.x-branch broke it. @HeartSaVioR if you get the chance could you take a look at it?

I'm of the opinion that it's too hard to enforce a maxUncommittedOffset globally for all partitions (we keep hitting edge cases where either the limit is ignored, or the spout is prevented from progressing), so I'd like to see us move to a per-partition limit (PR here #2156). That PR proposes moving numUncommittedOffsets into the OffsetManager, which means it automatically gets nulled when a partition is reassigned.

Basically I'd like to see if we can get #2156 discussed first. If we decide not to merge that one, then I think this is probably a good change, and I'd be happy to test this out then.

[WolfeeTJ]

@srdo I agree it would be a better solution to maintain uncommitted as per partition basis, it would be much clearer to debug. Thank you very much and looking forward to merging in master or branch 1.1.x.

[HeartSaVioR]

@srdo
Thanks for noticing.
Unfortunately #2190 seemed to touch diverged thing between 1.x-branch (1.2.0-SNAPSHOT) and 1.1.x-branch (1.1.1-SNAPSHOT). I was a bit careless at merging phase while cherry-picking.
I'll see how I can fix the thing, or just revert it only from 1.1.x-branch if it's hard to fix.

[srdo]

@HeartSaVioR Thanks. The extra parameter it's complaining about on ZkCoordinator is the task id added in #2188. I think you can just remove it from the test in 1.1.x and it'll work.

[HeartSaVioR]

@srdo Thanks I just fixed and pushed the patch to 1.1.x-branch without additional PR (I could fix it while merging, so I feel new review process is not needed.)