Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KAFKA-7277: Migrate Streams API to Duration instead of longMs times #5682
KAFKA-7277: Migrate Streams API to Duration instead of longMs times #5682
Changes from 15 commits
0293466
6724276
9730461
c7b3c9a
c4f2335
a6d47d4
b44aed9
52c5a84
4e42ed4
e5b9e5f
527f6fe
6b25162
d63b729
8d40a40
5dbe795
518a914
6eb48e3
f241d3a
4e8b65f
a583259
5210f9f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@guozhangwang @bbejeck @vvcephei Should we change this to "zero or negative" ? Comparing the implementation, passing in a negative timestamp will "expire" the timeout even without checking the state transition at all and return immediately (with
false
) even if the state transition was successful.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with those semantics, maybe we can make a Jira.
Thinking about it more, though, wouldn't it make more sense to:
ofYears(1)
orofMillis(Long.MAX_VALUE)
or some other intuitively "long enough to be forever" value instead of a magic value.Regardless, I agree the current behavior is a little weird, and I'd be in favor of a Jira/KIP to revise it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I'm ok with the semantics.
But my first instinct of a timeout of
0
implies shutdown immediately with no wait and blocking forever takes a value ofLong.MAX_VALUE
.In short, I'm +1 as well on revising the behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created: https://issues.apache.org/jira/browse/KAFKA-7477
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, we should fix this in 2.1 -- I am suggestion this, because we could use the new semantics for
close(Duration)
only and stay with old semantics forclose(long, TimeUnit)
-- if we don't so this, it would be an backward incompatible change and thus we could only do it in3.0.0.
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nizhikov Are you willing for address 7477 in this PR? If not, it's also fine and we do a follow up PR. However, it should be part of the KIP description. Could you update the KIP accordingly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mjsax I'll take care of KAFKA-7477 in follow up PR.
KIP-358 updated. Please, see, "Proposed Changes" section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you start working on KAFKA-7477 already @nizhikov ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can understand thus fix is trivial.
I'm planning to provide PR in a 24 hour after this PR finish. Is it OK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't call it trivial, but sure, this sound good! Thanks a lot. Just want to make sure we get it on time to not miss code freeze deadline. Thanks a lot!