[BEAM-121] Add display data to windowing transforms #124
Conversation
| public void populateDisplayData(DisplayData.Builder builder) { | ||
| builder | ||
| .add("numMonths", number) | ||
| .add("startDate", new DateTime(startDate, timeZone).toInstant()); |
There was a problem hiding this comment.
This could benefit from the "omit default" logic (I believe we use the epoch as the default).
swegner
force-pushed
the
displaydata-window
branch
4 times, most recently
from
526bf2a to
370d229
Compare
|
I've addressed all feedback so far. Please take another look. @bjchambers |
|
|
||
| @Override | ||
| public String toString() { | ||
| return "Repeatedly.forever(AfterWatermark.pastEndOfWindow)"; |
There was a problem hiding this comment.
This should be either:
"DefaultTrigger" (which may be what users expect to see, and lets us differentiate the two)
or it should reuse AfterWatermark.TO_STRING and such (specifically, your toString there had pastEndOfWindow()).
There was a problem hiding this comment.
Actually, what if we just omit display data for the default trigger (and leave the toString() implementation as-is). This is consistent with other places where we exclude defaults.
|
Some small comments, then I think this will LGTM |
|
I've addressed all feedback so far. Please take another look. @bjchambers |
| public class AfterWatermark { | ||
|
|
||
| private static final String TO_STRING = "AfterWatermark.pastEndOfWindow()"; | ||
| static final String TO_STRING = "AfterWatermark.pastEndOfWindow()"; |
There was a problem hiding this comment.
Since you're treating DefaultTrigger as default, you shouldn't need this to be visible, right?
swegner
force-pushed
the
displaydata-window
branch
from
c551412 to
adf6d1c
Compare
|
I've addressed all feedback so far. Please take another look. @bjchambers |
|
LGTM. Merged. |
|
Backported via GoogleCloudPlatform/DataflowJavaSDK#216 |
3 participants
Be sure to do all of the following to help us incorporate your contribution
quickly and easily:
[BEAM-<Jira issue #>] Description of pull requestmvn clean verify. (Even better, enableTravis-CI on your fork and ensure the whole test matrix passes).
number, if there is one.
Individual Contributor License Agreement.