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.

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

[STORM-2455] Expose the window start and end timestamp in TupleWindow #2053

Merged
merged 1 commit into from
Apr 13, 2017

Conversation

arunmahadevan
Copy link
Contributor

No description provided.

@harshach
Copy link
Contributor

@arunmahadevan this looks like backward incompatible change?

@arunmahadevan
Copy link
Contributor Author

@harshach, the earlier change (that added getTimestamp) was checked in to only the master branch. Since its not part of any released versions of Storm may be its not an issue if we can get this merged before the 2.0 release?

@@ -47,9 +47,17 @@
List<T> getExpired();

/**
* If processing based on event time, returns the watermark time otherwise the current timestamp.
* If processing based on event time, returns the window end time based on watermark otherwise
* returns the current timestamp.
Copy link
Member

Choose a reason for hiding this comment

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

Returning the current timestamp(at which this method is invoked) for non event-timestamp scenarios does not look to be intuitive. Users may assume endTimeStamp should return the timestamp at which the window is completed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for noticing. Its actually returning the timestamp at which window ends for both event and processing time. I am invoking System.currentTimeMillis in getEndTimestamp which is un-necessary. Will reword the doc and refactor a bit to make it clear.

@harshach
Copy link
Contributor

+1

@satishd
Copy link
Member

satishd commented Apr 12, 2017

+1 LGTM

@asfgit asfgit merged commit bf0f9bb into apache:master Apr 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants