You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by jerrypeng <gi...@git.apache.org> on 2017/09/08 17:45:10 UTC
[GitHub] storm pull request #2314: [STORM-2731] - Simple checks in Storm Windowing
GitHub user jerrypeng opened a pull request:
https://github.com/apache/storm/pull/2314
[STORM-2731] - Simple checks in Storm Windowing
There is also inconsistent and mixed used of Longs and Ints throughout the windowing code. Perhaps we should just change all the numeric values to use longs? Especially since all calculation of time is done in milliseconds. It would also be nice to make clear in both the code and documentation that all timestamps and time based calculations is in milliseconds.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jerrypeng/storm STORM-2731
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2314.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2314
----
commit 40d133b6c9f434eabc3b50bb8f4ebf91d5999930
Author: Jerry Peng <je...@jerrys-macbook-pro.local>
Date: 2017-09-07T22:32:39Z
[STORM-2731] - Simple checks in Storm Windowing
----
---
[GitHub] storm pull request #2314: [STORM-2731] - Simple checks in Storm Windowing
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng closed the pull request at:
https://github.com/apache/storm/pull/2314
---
[GitHub] storm issue #2314: [STORM-2731] - Simple checks in Storm Windowing
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on the issue:
https://github.com/apache/storm/pull/2314
@srdo I totally agree with your idea. I looked into changing all time variables to be long and renaming all variables to have a "Ms". It will probably be a significant diff
---
[GitHub] storm issue #2314: [STORM-2731] - Simple checks in Storm Windowing
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2314
@jerrypeng +1 for the changes in this PR. It's up to you whether you'd like to do the type changes + renames in this PR or a new one.
Please squash to one commit when you think this PR is complete. Thanks for the contribution.
---
[GitHub] storm issue #2314: [STORM-2731] - Simple checks in Storm Windowing
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on the issue:
https://github.com/apache/storm/pull/2314
@srdo thanks for the review. Lets do the larger change in a separate PR.
---
[GitHub] storm pull request #2314: [STORM-2731] - Simple checks in Storm Windowing
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2314#discussion_r139814647
--- Diff: storm-client/src/jvm/org/apache/storm/topology/base/BaseWindowedBolt.java ---
@@ -339,6 +364,9 @@ public BaseWindowedBolt withLag(Duration duration) {
* @param interval the interval at which watermark events are generated
*/
public BaseWindowedBolt withWatermarkInterval(Duration interval) {
+ if (interval == null) {
+ throw new IllegalArgumentException("watermark interval cannot be set null");
--- End diff --
Nit: First letter is lowercase
---
[GitHub] storm issue #2314: [STORM-2731] - Simple checks in Storm Windowing
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2314
I agree, we should use long for time unit parameters.
A decent way to make clear that timestamps are in milliseconds is to name the relevant variables so it is obvious, e.g. `timeMs` instead of `time`.
---
[GitHub] storm issue #2314: [STORM-2731] - Simple checks in Storm Windowing
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2314
Thanks @jerrypeng, merged to master. The windowing classes have moved since 1.x so please open another PR if you want this to go in those versions too.
---
[GitHub] storm pull request #2314: [STORM-2731] - Simple checks in Storm Windowing
Posted by jerrypeng <gi...@git.apache.org>.
GitHub user jerrypeng reopened a pull request:
https://github.com/apache/storm/pull/2314
[STORM-2731] - Simple checks in Storm Windowing
There is also inconsistent and mixed used of Longs and Ints throughout the windowing code. Perhaps we should just change all the numeric values to use longs? Especially since all calculation of time is done in milliseconds. It would also be nice to make clear in both the code and documentation that all timestamps and time based calculations is in milliseconds.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jerrypeng/storm STORM-2731
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2314.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2314
----
commit a64199ac5b5eb59205241ff09cd1d74440ba922f
Author: Jerry Peng <je...@jerrys-macbook-pro.local>
Date: 2017-09-07T22:32:39Z
[STORM-2731] - Simple checks in Storm Windowing
commit fb7c7a0ff4496d60ed5742702945090ba7c5674a
Author: Jerry Peng <je...@jerrys-macbook-pro.local>
Date: 2017-09-19T20:46:32Z
fixing exception message
commit f657dfcfadb74c967d4ef8bf711e4c27af6ccf45
Author: Jerry Peng <je...@jerrys-macbook-pro.local>
Date: 2017-09-19T20:55:08Z
fixing lower case
----
---
[GitHub] storm pull request #2314: [STORM-2731] - Simple checks in Storm Windowing
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2314#discussion_r139811727
--- Diff: storm-client/src/jvm/org/apache/storm/topology/base/BaseWindowedBolt.java ---
@@ -339,6 +364,9 @@ public BaseWindowedBolt withLag(Duration duration) {
* @param interval the interval at which watermark events are generated
*/
public BaseWindowedBolt withWatermarkInterval(Duration interval) {
+ if (interval == null) {
+ throw new IllegalArgumentException("Lag duration cannot be set null");
--- End diff --
This seems like it belongs in the withLag method.
---
[GitHub] storm issue #2314: [STORM-2731] - Simple checks in Storm Windowing
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2314
Also opened an issue for tracking the int -> long change and variable renames so we don't forget https://issues.apache.org/jira/browse/STORM-2747.
---
[GitHub] storm pull request #2314: [STORM-2731] - Simple checks in Storm Windowing
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2314
---