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

[BEAM-1327] Replace OutputTimeFn UDF with TimestampCombiner enum #2683

Merged
merged 1 commit into from Apr 26, 2017
Merged

[BEAM-1327] Replace OutputTimeFn UDF with TimestampCombiner enum #2683

merged 1 commit into from Apr 26, 2017

Conversation

kennknowles
Copy link
Member

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

  • Make sure the PR title is formatted like:
    [BEAM-<Jira issue #>] Description of pull request
  • Make sure tests pass via mvn clean verify. (Even better, enable
    Travis-CI on your fork and ensure the whole test matrix passes).
  • Replace <Jira issue #> in the title with the actual Jira issue
    number, if there is one.
  • If this contribution is large, please file an Apache
    Individual Contributor License Agreement.

@kennknowles
Copy link
Member Author

An actual important change that occurred here is this:

  • Previously, the WindowingStrategy combined the OutputTimeFn with the WindowFn to make a new custom OutputTimeFn that shifted timestamps as needed.
  • Now that it is an enum, this cannot be done. So now the runner has to be sure to pass all input timestamps through WindowFn.getOutputTime(...) before using the timestamp combiner.

@kennknowles
Copy link
Member Author

R: @tgroh since you are on a roll reviewing epic renames :-)

@jasonkuster
Copy link
Contributor

Gonna try to see if Jenkins is back

@jasonkuster
Copy link
Contributor

retest this please

tgroh
tgroh approved these changes Apr 25, 2017
View reviewed changes
...uction-java/src/main/java/org/apache/beam/runners/core/construction/WindowingStrategies.java Outdated
return TimestampCombiner.END_OF_WINDOW;
case LATEST_IN_PANE:
return TimestampCombiner.LATEST;
default:
Copy link
Member

Choose a reason for hiding this comment

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

case UNRECOGNIZED? otherwise comment is a bit iffy.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

sdks/java/core/src/main/java/org/apache/beam/sdk/testing/WindowFnTestUtils.java Outdated
@@ -321,4 +323,33 @@ public void merge(Collection<W> toBeMerged, W mergeResult) throws Exception {
earlierEndingWindow = window;
}
}

private static Instant assignOutputTime(TimestampCombiner timestampCombiner, Instant inputTimestamp,
Copy link
Member

Choose a reason for hiding this comment

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

formatting?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@kennknowles
Copy link
Member Author

retest this please

kennknowles
kennknowles commented Apr 26, 2017
View reviewed changes
Copy link
Member Author

@kennknowles kennknowles 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 catches. I'll squash and verify thoroughly before pushing.

...uction-java/src/main/java/org/apache/beam/runners/core/construction/WindowingStrategies.java Outdated
return TimestampCombiner.END_OF_WINDOW;
case LATEST_IN_PANE:
return TimestampCombiner.LATEST;
default:
Copy link
Member Author

Choose a reason for hiding this comment

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

Done

sdks/java/core/src/main/java/org/apache/beam/sdk/testing/WindowFnTestUtils.java Outdated
@@ -321,4 +323,33 @@ public void merge(Collection<W> toBeMerged, W mergeResult) throws Exception {
earlierEndingWindow = window;
}
}

private static Instant assignOutputTime(TimestampCombiner timestampCombiner, Instant inputTimestamp,
Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@kennknowles kennknowles force-pushed the deprecate-OutputTimeFn branch 10 times, most recently from e20137d to 71f965b Compare April 26, 2017 21:03
@kennknowles
Replace OutputTimeFn UDF with TimestampCombiner enum
f38e427 
This also removes the last dependency from SDK core to Runner API proto.
@asfgit asfgit merged commit f38e427 into apache:master Apr 26, 2017
asfgit pushed a commit that referenced this pull request Apr 26, 2017
@kennknowles
This closes #2683: Replace OutputTimeFn UDF with TimestampCombiner enum
a32371e
@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 69.901% when pulling f38e427 on kennknowles:deprecate-OutputTimeFn into 7339882 on apache:master.

@dhalperi dhalperi mentioned this pull request Apr 27, 2017
Closed
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 69.774% when pulling f38e427 on kennknowles:deprecate-OutputTimeFn into 7339882 on apache:master.

@kennknowles kennknowles deleted the deprecate-OutputTimeFn branch May 26, 2017 05:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgroh tgroh tgroh approved these changes

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@kennknowles @jasonkuster @coveralls @tgroh @asfgit