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 4 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.
Feel free to add a static import for
ofMillis
andSTREAMS_TIME
to shorten this line.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.
This is a public package and thus would make this class public API -- don't think we want this (also not part of the KIP). Maybe, we should create a new package
org.apache.kafka.streams.internals
and move it there?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.
moved
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 am wondering, what we gain by introducing this class?
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.
Hello @mjsax
As discussed in KIP thread we want to wrap NPE and
ArithmeticException
intoIllegalArgumentException
See @vvcephei suggestion: "I still feel that surfacing the ArithmeticException directly would not be a great experience, so I still advocate for wrapping it in an IllegalArgumentException that explains our upper bound for Duration is "max-long number of milliseconds"
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.
Please avoid single-letter variables:
d
->duration
(also below)
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.
This validation doesn't actually check that it's non-negative. I'm not sure that we want to, though.
Also, I'd use
Objects.toString()
onname
just in case it's null.Finally, it might be nice to wrap the substituted variables with brackets to make the result more readable.
All together, this would yield:
"[" + Objects.toString(name) + "] shouldn't be null.", e);
"[" + Objects.toString(name) + "] is too big to be converted to milliseconds: [" + d + "]", e);
What do you think about this?
(also below)
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.
We should use
validateMillisecondDuration
here.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.
This method is actually guaranteed not to throw this exception. Since the parameter is a
long
of milliseconds, it's not possible to pass a value that is not expressible in milliseconds as along
.(also applies elsewhere)
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 I mentioned before, we don't throw
IAE
iftimeDifference
is negative.(also below)