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

[BEAM-1645] Populate display data on Window.Assign #2189

Closed
wants to merge 1 commit into from

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

@tgroh @bjchambers I haven't actually run this yet and looked at the UI, but you might be interested.

}
populateWindowingStrategyDisplayData(
WindowingStrategy.of(windowFn)
.withAllowedLateness(allowedLateness)
Copy link
Member Author

Choose a reason for hiding this comment

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

Here I need to build the windowing strategy conditionally.

Copy link
Member

Choose a reason for hiding this comment

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

I feel as though allowing a null WindowFn should not be permitted by WindowingStrategy, as an aside.

Copy link
Member

Choose a reason for hiding this comment

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

We do lack the context here to construct the entire windowing strategy, which is unfortunate. Window.Assign is provided it on construction, but I don't know if there's any way to pass it to the identity flatten's display data.

private static void populateWindowingStrategyDisplayData(
WindowingStrategy<?, ?> windowingStrategy, DisplayData.Builder builder) {

builder
Copy link
Member Author

Choose a reason for hiding this comment

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

The windowFn and closing behavior are not conditionalized the same way. Cleaner would be to explicitly represent a diff vs a completed strategy.

Copy link
Member

@tgroh tgroh left a comment

Choose a reason for hiding this comment

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

This looks like mostly the right approach. It may have to be that we have the "populate with the following n fields" instead of having a smarter thing that populates from the entire WindowingStrategy, just by virtue of the lack of entire symmetry with "isWindowFnSpecified"

}
populateWindowingStrategyDisplayData(
WindowingStrategy.of(windowFn)
.withAllowedLateness(allowedLateness)
Copy link
Member

Choose a reason for hiding this comment

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

I feel as though allowing a null WindowFn should not be permitted by WindowingStrategy, as an aside.

}
populateWindowingStrategyDisplayData(
WindowingStrategy.of(windowFn)
.withAllowedLateness(allowedLateness)
Copy link
Member

Choose a reason for hiding this comment

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

We do lack the context here to construct the entire windowing strategy, which is unfortunate. Window.Assign is provided it on construction, but I don't know if there's any way to pass it to the identity flatten's display data.

@asfbot
Copy link

asfbot commented Mar 8, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8188/

Build result: FAILURE

[...truncated 726.57 KB...] ^/home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/runners/core-java/src/main/java/org/apache/beam/runners/core/triggers/TriggerStateMachine.java:198: error: reference not found * Removes the timer set in this trigger context for the given {@link Instant} ^Command line was: /usr/local/asfpackages/java/jdk1.8.0_121/jre/../bin/javadoc @options @packagesRefer to the generated Javadoc files in '/home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/runners/core-java/target/apidocs' dir. at org.apache.maven.plugin.javadoc.AbstractJavadocMojo.executeJavadocCommandLine(AbstractJavadocMojo.java:5188) at org.apache.maven.plugin.javadoc.AbstractJavadocMojo.executeReport(AbstractJavadocMojo.java:2075) at org.apache.maven.plugin.javadoc.JavadocJar.execute(JavadocJar.java:188) ... 33 more2017-03-08T00:48:59.226 [ERROR] 2017-03-08T00:48:59.226 [ERROR] Re-run Maven using the -X switch to enable full debug logging.2017-03-08T00:48:59.227 [ERROR] 2017-03-08T00:48:59.227 [ERROR] For more information about the errors and possible solutions, please read the following articles:2017-03-08T00:48:59.227 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException2017-03-08T00:48:59.227 [ERROR] 2017-03-08T00:48:59.227 [ERROR] After correcting the problems, you can resume the build with the command2017-03-08T00:48:59.227 [ERROR] mvn -rf :beam-runners-core-javachannel stoppedSetting status of b3c979f to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8188/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install
--none--

@tgroh
Copy link
Member

tgroh commented Mar 17, 2017

This is probably preempted by #2258

@tgroh
Copy link
Member

tgroh commented Mar 23, 2017

@kennknowles is this still worthwhile?

@kennknowles
Copy link
Member Author

I was going to ask you the same thing. I think it was addressed adequately while I was away.

@kennknowles kennknowles deleted the Assign-DisplayData branch March 31, 2017 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants