Skip to content
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.

Sign up for GitHub

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

Jump to bottom

KAFKA-7446: Fix the duration and instant validation messages #5930

Merged
merged 1 commit into from
Dec 4, 2018
Merged

KAFKA-7446: Fix the duration and instant validation messages #5930

merged 1 commit into from
Dec 4, 2018

Conversation

mrsrinivas
Copy link
Contributor

@mrsrinivas mrsrinivas commented Nov 19, 2018 •
edited
Loading

Changes made as part of this PR for KAFKA-7446

  • Improved error message for better readability
  • Corrected java documentation on AdvanceInterval check.

Core and Streams modules' JUnit tests are successful in local but
tests related to file system is failed since I am running on Windows.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@mrsrinivas mrsrinivas changed the title [WIP] KAFKA-7446: Better error messages on WindowDuration and AdvanceInterval [WIP] KAFKA-7446: Better error messages on WindowDuration and AdvanceInterval Nov 19, 2018
@mrsrinivas mrsrinivas changed the title [WIP] KAFKA-7446: Better error messages on WindowDuration and AdvanceInterval [WIP] KAFKA-7446: Better error messages on Streams WindowDuration and AdvanceInterval Nov 19, 2018
@mjsax mjsax added the streams label Nov 19, 2018
mjsax
mjsax reviewed Nov 19, 2018
View reviewed changes
Copy link
Member

@mjsax mjsax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Couple of comments.

Note: build failed with checkstyle error; please fix.

streams/src/main/java/org/apache/kafka/streams/errors/InvalidAdvanceIntervalException.java Outdated Show resolved Hide resolved
streams/src/main/java/org/apache/kafka/streams/errors/InvalidWindowDurationException.java Outdated Show resolved Hide resolved
streams/src/main/java/org/apache/kafka/streams/internals/ApiUtils.java Outdated Show resolved Hide resolved
@mrsrinivas mrsrinivas changed the title [WIP] KAFKA-7446: Better error messages on Streams WindowDuration and AdvanceInterval KAFKA-7446: Better error messages on Streams WindowDuration and AdvanceInterval Nov 20, 2018
@mrsrinivas
Copy link
Contributor Author

ping @jaceklaskowski @guozhangwang. any thoughts about the changes?

TIA

mjsax
mjsax reviewed Nov 20, 2018
View reviewed changes
Copy link
Member

@mjsax mjsax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating the PR. Couple of follow ups.

streams/src/main/java/org/apache/kafka/streams/kstream/TimeWindows.java Outdated Show resolved Hide resolved
streams/src/main/java/org/apache/kafka/streams/kstream/TimeWindows.java Outdated Show resolved Hide resolved
streams/src/test/java/org/apache/kafka/streams/kstream/TimeWindowsTest.java Outdated Show resolved Hide resolved
@mrsrinivas mrsrinivas changed the title KAFKA-7446: Better error messages on Streams WindowDuration and AdvanceInterval KAFKA-7446: Better error message for window advance interval out of range Nov 21, 2018
@mrsrinivas mrsrinivas changed the title KAFKA-7446: Better error message for window advance interval out of range KAFKA-7446: Better handling of validation error messages Nov 22, 2018
@mrsrinivas mrsrinivas changed the title KAFKA-7446: Better handling of validation error messages KAFKA-7446: Better handling of time validation error messages Nov 22, 2018
@mrsrinivas
Copy link
Contributor Author

ping @mjsax @guozhangwang @bbejeck @vvcephei

@mrsrinivas mrsrinivas changed the title KAFKA-7446: Better handling of time validation error messages KAFKA-7446: Better handling of duration and instant validation messages Nov 23, 2018
@mrsrinivas mrsrinivas changed the title KAFKA-7446: Better handling of duration and instant validation messages KAFKA-7446: Improve duration and instant validation messages Nov 23, 2018
@mrsrinivas
Copy link
Contributor Author

retest this please

@mrsrinivas mrsrinivas changed the title KAFKA-7446: Improve duration and instant validation messages KAFKA-7446: Fix the duration and instant validation messages Nov 26, 2018
mjsax
mjsax reviewed Nov 27, 2018
View reviewed changes
Copy link
Member

@mjsax mjsax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. Some follow up nits.

streams/src/main/java/org/apache/kafka/streams/KafkaStreams.java Outdated Show resolved Hide resolved
streams/src/main/java/org/apache/kafka/streams/internals/ApiUtils.java Outdated Show resolved Hide resolved
streams/src/main/java/org/apache/kafka/streams/internals/ApiUtils.java Outdated Show resolved Hide resolved
@mrsrinivas
Copy link
Contributor Author

@mjsax, Thank you for the review comments.

I addressed all the comments.

mjsax
mjsax approved these changes Nov 28, 2018
View reviewed changes
Copy link
Member

@mjsax mjsax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Call for second review @jaceklaskowski @guozhangwang @bbejeck @vvcephei

bbejeck
bbejeck approved these changes Nov 28, 2018
View reviewed changes
Copy link
Contributor

@bbejeck bbejeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one minor nit otherwise LGTM.

I have one thought for a follow-on PR, we could consider moving the prepareMillisCheckFailMsgPrefix inside the validateXXX methods and create the formatted string at that point saving the users a method call.

But I think this PR is good enough as is.

streams/src/main/java/org/apache/kafka/streams/internals/ApiUtils.java Outdated Show resolved Hide resolved
@mrsrinivas
KAFKA-7446: Fix the duration and instant validation messages.
a0cd5ea 
Changes made as part of this commit.
 - Improved error message for better readability at millis validation utility
 - Corrected java documentation on `AdvanceInterval` check.
 - Added caller specific prefix text to make error message more clear to developers/users.
@mrsrinivas
Copy link
Contributor Author

Thank you @bbejeck and @mjsax for the review. I incorporated the suggested change.

@mrsrinivas
Copy link
Contributor Author

ping @mjsax @guozhangwang @bbejeck @vvcephei @jaceklaskowski

@guozhangwang guozhangwang merged commit 7283711 into apache:trunk Dec 4, 2018
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
@mrsrinivas @pengxiaolong
KAFKA-7446: Fix the duration and instant validation messages. (apache…
b91c395 
…#5930)

Changes made as part of this commit.
 - Improved error message for better readability at millis validation utility
 - Corrected java documentation on `AdvanceInterval` check.
 - Added caller specific prefix text to make error message more clear to developers/users.

Reviewers: Matthias J. Sax <matthias@confluent.io>, Bill Bejeck <bill@confluent.io>, Jacek Laskowski <jacek@japila.pl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaceklaskowski jaceklaskowski jaceklaskowski left review comments

@bbejeck bbejeck bbejeck approved these changes

@mjsax mjsax mjsax approved these changes

Assignees
No one assigned
Labels
streams
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mrsrinivas @jaceklaskowski @bbejeck @mjsax @guozhangwang