STORM-2607: Storm-kafka-client never commits the last message on a partition.#2367
Conversation
fd50c48 to
83de52b
Compare
| if the topic is compacted or deleted), the consumer should jump to | ||
| the next logical point in the topic. Next logical offset should be the | ||
| first element after committedOffset in the ascending ordered emitted set. | ||
| first element after nextEarliestUncommittedOffset in the ascending ordered emitted set. |
There was a problem hiding this comment.
The field nextEarliestUncommittedOffset looks removed in the patch and this comment refers it.
83de52b to
1d728a4
Compare
|
+1 |
| this.committedOffset = initialFetchOffset - 1; | ||
| LOG.debug("Instantiated {}", this); | ||
| this.committedOffset = initialFetchOffset; | ||
| LOG.debug("Instantiated {}", this.toString()); |
There was a problem hiding this comment.
Yes, but I get a warning about leaking this from the constructor, which this fixes.
There was a problem hiding this comment.
This is not a big deal, but can you tell me what kinda warning you are talking about? I never heard of such a thing. When you concatenate a string with an object reference, the toString() method of that object reference is called. I am not sure why both approaches are any different.
There was a problem hiding this comment.
The warning I get is just "leaking this in constructor", Netbeans has the following description: "Using this as parameter can be dangerous in the contructor because the object is not fully initialized.". IntelliJ and Eclipse have similar warnings I believe.
It's basically saying not to use this as a parameter from the constructor, because the called method may use fields that aren't initialized. I agree that leaking this from the constructor is not dangerous here, because sfl4j just calls toString, but I'd still like to get rid of the warning. The reason this is flagged is because this is used as a parameter to LOG.debug, and not as part of a string concatenation. The IDE can't tell whether this is used unsafely in LOG.debug, so it's flagged as a warning.
There was a problem hiding this comment.
Thanks for the explanation. I just noticed that this is called within its own constructor, and therefore makes sense. However, IntelliJ does not give me this warning.
There was a problem hiding this comment.
It's not enabled by default in IntelliJ. If you go to settings -> editor -> inspections and search for "this", there's one called "'this' reference escaped in object construction"
|
@HeartSaVioR No, it doesn't look like it. I'll put up a 1.x and 1.1.x version. |
|
@srdo I understand that you want to keep the commit history for @tiodollar but I don't think we should have 4 commits for such a simple change. This patch should be one commit only, either yours or @tiodollar 's . Can either you or @tiodollar please squash the commits. Thanks. |
|
@hmcl I agree that this many commits is not ideal. The issue is with being able to track who contributed what code. If I squash we can't tell which parts of this PR was contributed by me, and which were contributed by @tiodollar. It's my impression that we need to keep the history of who contributed what intact. |
|
@srdo in my opinion the most important thing is to keep the project, and in particular the git log clean. I often have to cherry-pick changes from Apache branch into my own branch, and multiple commits for a JIRA turn branch maintenance nightmare. Several Apache projects have a strict rule of one JIRA one Git commit, which I believe is very beneficial. If you feel really strongly about differentiating yours and his contributions, this patch must have at most two commits. One has all his contributions, and another yours. You can squash the commits and put @tiodollar as author as described in here. |
1d728a4 to
85d464a
Compare
|
@hmcl It's not that I feel strongly about keeping the contributions separate, but I think it's a rule for Apache projects that it must be possible to tell who contributed what, even historically. I can't recall where it says so though. Squashed to two commits. |
| LOG.debug("Committed [{}] offsets in the range [{}-{}] for topic-partition [{}].", | ||
| numCommittedOffsets, preCommitCommittedOffsets + 1, this.committedOffset, tp); | ||
| LOG.debug("Committed [{}] offsets in the range [{}-{}] for topic-partition [{}]." | ||
| + " Processing will resume at [{}] if the spout restarts.", |
There was a problem hiding this comment.
at offset [{}] in case of a spout restart.
… instead of last committed offset for compatibility with commitSync consumer API
85d464a to
d7bdc2d
Compare
|
@srdo are you on Gitter or some sort of chat room where I could ask you a few questions? Thanks. |
|
@hmcl Just signed in on Gitter, feel free to message me. |
|
I'll merge this weekend if I no one brings up more concerns. |
|
I read a mail at dev@ from @hmcl and the talk was not relevant to this patch. +1 again. |
|
Merged, thanks for the reviews guys. |
4 participants
This finishes up #2181, which has been blocked on a rebase for a while. It was suggested on the mailing list that we finish this up quickly since the fix should ideally go in 1.1.2.
I've squashed the commits as much as possible without mixing up which changes were made by @tiodollar and which were made by me. Let me know if squashing is not okay in this case, and I'll rebase on the commits in the original PR.