STORM-2546: Fix storm-kafka-client spout getting stuck when retriable offsets were deleted from the Kafka log due to topic compaction

[srdo]

This requires the changes in #2156 for correctness, please ignore the first commit.

Here's the idea behind these changes https://issues.apache.org/jira/browse/STORM-2546?focusedCommentId=16151172&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16151172

KafkaSpoutCommitTest was renamed, so the first test in KafkaSpoutLogCompactionSupportTest was already there and can be skipped.

[srdo]

@askprasanna I've made the spout default to an auto offset reset policy of "earliest" when the user requests the at-least-once processing guarantee. I think this addresses your request here https://issues.apache.org/jira/browse/STORM-2546?focusedCommentId=16152824&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16152824.

[askprasanna]

@srdo Awesome. Thanks for following up.

[HeartSaVioR]

+1
My only 2 cents is that would we want to open the possibility to break the spout by theirselves (I mean users)?
If my understanding of this patch is right, both AT_LEAST_ONCE and other than earliest shouldn't be set. If I'm right we may want to restrict it with showing error message sooner, or at least expose warn message.

[HeartSaVioR]

@srdo
I found same suggestion as my 2 cents is already discussed from JIRA issue. Is it already addressed?

[srdo]

@HeartSaVioR Yes, you are right. We set the auto offset reset policy to earliest by default when AT_LEAST_ONCE is picked if the user hasn't explicitly set anything else.

The misconfiguration I'm most worried about is where users set AT_LEAST_ONCE, and then forget to set the auto offset reset policy which defaults to latest, because it's an easy mistake to make if you're not already very familiar with Kafka's options. The other weird configurations (AT_LEAST_ONCE+latest, AT_MOST_ONCE+earliest) have to be explicitly chosen by the user.

I can't think of a reason why users would want to use those configurations, but I thought it might be better not to prevent the user from using those settings if they really want to, because there might be a use case I'm not seeing.

I'm happy to add in checks and error messages (or maybe even throwing exceptions) when using those configurations, if you think it makes sense?

[HeartSaVioR]

@srdo
Sure. Makes sense. Let's add a warning message.

[srdo]

@HeartSaVioR Added the warning. Will squash and merge if the warning looks good.

[HeartSaVioR]

@srdo +1
Thanks for addressing quickly. Please go ahead squashing and merging.
Btw, could we have PR for 1.x branch as well?

[srdo]

@HeartSaVioR Merged to master. Yes, I'll put up a 1.x version tonight.