[BEAM-1148] Port PAssert away from Aggregators by pabloem · Pull Request #2291 · apache/beam

pabloem · 2017-03-22T22:01:14Z

This PR contains the following changes:

Changing PAssert.java code to use Metrics instead of Aggregators
Changing TestSparkRunner.java code to use Metrics. The change here also required adding up different Metrics counters because Metrics are per-step; while Aggregators in Spark seem to be adding accross steps.

Additional bug fixes/code improvements:

Fixing a bug in MetricFiltering
Changing the Spark runner to use MetricFiltering instead of its own filtering implementation - if that's okay : )
I ran mvn clean verify successfully.

Are there other places where I should be porting Aggregators checks to Metrics?

asfbot · 2017-03-22T22:12:26Z

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

Build result: FAILURE

[...truncated 823.52 KB...] at hudson.remoting.UserRequest.perform(UserRequest.java:153) at hudson.remoting.UserRequest.perform(UserRequest.java:50) at hudson.remoting.Request$2.run(Request.java:336) at hudson.remoting.InterceptingExecutorService$1.call(InterceptingExecutorService.java:68) at java.util.concurrent.FutureTask.run(FutureTask.java:266) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) at java.lang.Thread.run(Thread.java:745)Caused by: org.apache.maven.plugin.MojoFailureException: You have 1 Checkstyle violation. at org.apache.maven.plugin.checkstyle.CheckstyleViolationCheckMojo.execute(CheckstyleViolationCheckMojo.java:588) at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:134) at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:208) ... 31 more2017-03-22T22:12:18.435 [ERROR] 2017-03-22T22:12:18.435 [ERROR] Re-run Maven using the -X switch to enable full debug logging.2017-03-22T22:12:18.435 [ERROR] 2017-03-22T22:12:18.435 [ERROR] For more information about the errors and possible solutions, please read the following articles:2017-03-22T22:12:18.435 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException2017-03-22T22:12:18.435 [ERROR] 2017-03-22T22:12:18.435 [ERROR] After correcting the problems, you can resume the build with the command2017-03-22T22:12:18.435 [ERROR] mvn -rf :beam-runners-core-javachannel stoppedSetting status of 1234252 to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8675/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install
--none--

asfbot · 2017-03-22T22:36:11Z

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

Build result: FAILURE

[...truncated 849.56 KB...] at hudson.remoting.UserRequest.perform(UserRequest.java:153) at hudson.remoting.UserRequest.perform(UserRequest.java:50) at hudson.remoting.Request$2.run(Request.java:336) at hudson.remoting.InterceptingExecutorService$1.call(InterceptingExecutorService.java:68) at java.util.concurrent.FutureTask.run(FutureTask.java:266) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) at java.lang.Thread.run(Thread.java:745)Caused by: org.apache.maven.plugin.MojoFailureException: You have 1 Checkstyle violation. at org.apache.maven.plugin.checkstyle.CheckstyleViolationCheckMojo.execute(CheckstyleViolationCheckMojo.java:588) at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:134) at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:208) ... 31 more2017-03-22T22:36:01.904 [ERROR] 2017-03-22T22:36:01.904 [ERROR] Re-run Maven using the -X switch to enable full debug logging.2017-03-22T22:36:01.904 [ERROR] 2017-03-22T22:36:01.904 [ERROR] For more information about the errors and possible solutions, please read the following articles:2017-03-22T22:36:01.905 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException2017-03-22T22:36:01.905 [ERROR] 2017-03-22T22:36:01.905 [ERROR] After correcting the problems, you can resume the build with the command2017-03-22T22:36:01.905 [ERROR] mvn -rf :beam-runners-core-javachannel stoppedSetting status of 9711009 to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8676/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install
--none--

coveralls · 2017-03-22T23:25:52Z

Changes Unknown when pulling 5dcedaa on pabloem:fix-passert into ** on apache:master**.

asfbot · 2017-03-22T23:27:43Z

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

tgroh

Test(Apex/Dataflow/Flink)Runner are likely to all be broken after this change, unless there's an equivalent change to get them to use Metrics

Can you split the bug fixes out into a separate PR?

tgroh · 2017-03-23T01:00:01Z

runners/spark/src/main/java/org/apache/beam/runners/spark/TestSparkRunner.java

+                MetricNameFilter.named(PAssert.class, PAssert.SUCCESS_COUNTER)).build());
+        long successAssertions = 0;
+        for (MetricResult<Long> res : queryResults.counters()) {
+          successAssertions += res.attempted().longValue();


Successes should be logical. A successful PAssert should be committed exactly once.

The Spark runner does not support committed metrics. @aviemzur what do you think? We could merge this as is, and file a bug to change it once committed metrics are supported.

Committed metrics will probably not be supported in Spark runner.

tgroh · 2017-03-23T01:01:14Z

runners/spark/src/main/java/org/apache/beam/runners/spark/TestSparkRunner.java

+                MetricNameFilter.named(PAssert.class, PAssert.FAILURE_COUNTER)).build());
+        long failedAssertions = 0;
+        for (MetricResult<Long> res : queryResults.counters()) {
+          failedAssertions += res.attempted().longValue();


(This one should remain attempted - we should never commit the failure, so it should be impossible for the failure counter to ever go above zero in committed - but this does generically mean that we can't be guaranteed to have a completely accurate signal of failed assertions.)

aviemzur · 2017-03-23T03:55:43Z

I think we should consider instead to move on https://issues.apache.org/jira/browse/BEAM-1763 and remove these assertions from TestSparkRunner. This will also remove the need for #2263

tgroh · 2017-03-23T16:57:37Z

Tests (especially those with multiple assertions) won't reliably work in the TestSparkRunner if that's done, though. This design roughly matches up with my understanding of how we would want to query PAssert success metrics, so if we can't support it in Spark we can't really write BEAM-1763 in a runner-independent manner that actually works on all of our runners.

aviemzur · 2017-03-23T17:02:37Z

Not with metrics, no. But @bjchambers had an idea for doing this without metrics, but with sideoutput instead.

pabloem · 2017-03-23T17:29:18Z

That's fair. I'll talk to Ben to figure out how to do this with side outputs. I'll submit the bug fix in a different PR.

pab-goog added 2 commits March 22, 2017 15:27

Tentative fixing PAssert for Spark runner tests.

7f2456f

Fixing lint issue

9711009

pabloem force-pushed the fix-passert branch from 1234252 to 9711009 Compare March 22, 2017 22:27

Fixing lint issue.

5dcedaa

tgroh reviewed Mar 23, 2017

View reviewed changes

pabloem closed this Mar 23, 2017

Conversation

pabloem commented Mar 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asfbot commented Mar 22, 2017

Build result: FAILURE

Uh oh!

asfbot commented Mar 22, 2017

Build result: FAILURE

Uh oh!

coveralls commented Mar 22, 2017

Uh oh!

asfbot commented Mar 22, 2017

Uh oh!

tgroh left a comment

Choose a reason for hiding this comment

Uh oh!

tgroh Mar 23, 2017

Choose a reason for hiding this comment

Uh oh!

pabloem Mar 23, 2017

Choose a reason for hiding this comment

Uh oh!

aviemzur Mar 23, 2017

Choose a reason for hiding this comment

Uh oh!

tgroh Mar 23, 2017

Choose a reason for hiding this comment

Uh oh!

aviemzur commented Mar 23, 2017

Uh oh!

tgroh commented Mar 23, 2017

Uh oh!

aviemzur commented Mar 23, 2017

Uh oh!

pabloem commented Mar 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Comments

pabloem commented Mar 22, 2017 •

edited

Loading