[BEAM-160] Port 'NexMark Queries' to Beam for use as integration test #3114

echauchot · 2017-05-12T08:45:46Z

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.
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.

R: @dpmills @dhalperi
CC: @stasl @aviemzur @aljoscha because we discussed NexMark together :)
CC: @mshields822 I know you don not work on it anymore, but you might be interested :)
CC: @ssisk for reflexion for IT tests

This is a port of the NexMark queries to Beam, to be used as integration tests.
This can also be used as A-B testing (no-regression or performance comparison between 2 versions of the same engine or of the same runner)

This a continuation of the previous PRs (#99 and #366) from Mark Shields.
The code has changed quite a bit: some queries have changed to use new Beam APIs and there where some big refactorings. More important, we can now run all the queries in all the runners. Nevertheless, there are still some open issues in Nexmark (https://github.com/iemejia/beam/issues) and in Beam upstream (see issue links in https://issues.apache.org/jira/browse/BEAM-160)

Here is a doc that present NexMark components and pseudo code of the queries to ease the review : https://drive.google.com/open?id=1VgnGiVu8vSfm7Et-xAtQYv0PlEpqeyfmhpQUNPmWRJs

Everything needed to launch the queries is in the Readme. There is also a support matrix towards the runners.

Please do not squash commits because there are several authors Mark, Ismaël and I.

Good review :) !

coveralls · 2017-05-12T09:53:47Z

Coverage decreased (-2.07%) to 68.13% when pulling 05698df on iemejia:BEAM-160-nexmark into d6ac39a on apache:master.

jbonofre · 2017-05-12T13:24:48Z

R: @jbonofre

dhalperi · 2017-05-23T16:33:32Z

R: -@dhalperi

dpmills · 2017-05-24T19:00:09Z

integration/java/nexmark/src/main/java/org/apache/beam/integration/nexmark/NexmarkLauncher.java

+    return options.isStreaming();
+  }
+
+  /**


Why are these hard-coded?

This option allows the user to specify if we wants to run the pipelines in streaming mode or in batch mode. If he sets streaming=true in the options, then the first step of the pipelines will be
Read.from(new UnboundedEventSource) otherwise it will be Read.from(new BoundedEventSource)

Sorry for the unclear comment; I was referring to the coresPerWorker and maxNumWorkers() functions

IMHO I think they are leftovers: coresPerWorker is never used, so I will remove it. And maxNumWorkers() is only used in query 10 (write event logs to Google Cloud Storage) to define the number of shards.

I will start a discussion on the ML about whether we should keep query 10 like this (with Google specifics as the aim was to test a specific Google customer use case) or use a more generic (Beam filesystems) data storage technology. If so, the configuration parameter is likely to disappear also. But this refactoring of query 10 does not block the PR I think. It could be done afterwards.

dpmills · 2017-05-24T19:01:15Z

integration/java/nexmark/src/main/java/org/apache/beam/integration/nexmark/NexmarkLauncher.java

+   */
+  private void invokeBuilderForPublishOnlyPipeline(PipelineBuilder<NexmarkOptions> builder) {
+    builder.build(options);
+//    throw new UnsupportedOperationException(


Delete commented code if it's not needed

Yes .The PR in general lacks a big cleaning :)

dpmills · 2017-05-24T19:02:26Z

integration/java/nexmark/src/main/java/org/apache/beam/integration/nexmark/NexmarkLauncher.java

+   * If monitoring, wait until the publisher pipeline has run long enough to establish
+   * a backlog on the Pubsub topic. Otherwise, return immediately.
+   */
+  private void waitForPublisherPreload() {


Consider just deleting this along with the other preload-related functionality, since it doesn't work in this PR

I only left conf.preloadSeconds that is used to count initialization time in the time before cancelling the job.

dpmills · 2017-05-24T19:06:00Z

integration/java/nexmark/src/main/java/org/apache/beam/integration/nexmark/NexmarkLauncher.java

+          sinkEventsToAvro(source);
+        }
+
+        // Special hacks for Query 10 (big logger).


Add more comments about what the hacks are and why they are needed

dpmills · 2017-05-24T19:08:14Z

integration/java/nexmark/src/main/java/org/apache/beam/integration/nexmark/NexmarkUtils.java

+   * Setup pipeline with codes and some other options.
+   */
+  public static void setupPipeline(CoderStrategy coderStrategy, Pipeline p) {
+    //TODO Ismael check


The new direct runner doesn't support disabling randomization at all, so this can be deleted. Do the tests pass on DirectRunner without it?

Yes, all the queries and and all the unit tests pass in direct runner in batch and in streaming mode even if randomization is not disabled so I can safely remove this piece of code.

dpmills · 2017-05-24T19:09:19Z

integration/java/nexmark/src/main/java/org/apache/beam/integration/nexmark/NexmarkUtils.java

+  /**
+   * Return a transform to write given number of bytes to durable store on every record.
+   */
+  public static <T> ParDo.SingleOutput<T, T> diskBusy(String name, final long bytes) {


This can use the State API now

Yes, forgot that one, thanks!

dpmills · 2017-05-24T19:11:30Z

integration/java/nexmark/src/main/java/org/apache/beam/integration/nexmark/model/Event.java

+    public void encode(Event value, OutputStream outStream)
+        throws CoderException, IOException {
+      if (value.newPerson != null) {
+        INT_CODER.encode(0, outStream);


Put the tag values in an enum

Missed that one also, thanks!

coveralls · 2017-05-30T15:53:38Z

Coverage decreased (-1.6%) to 68.55% when pulling 403f1ec on iemejia:BEAM-160-nexmark into d6ac39a on apache:master.

asfbot · 2017-05-30T15:58:34Z

--none--

asfbot · 2017-05-31T08:17:26Z

Build finished.
--none--

echauchot · 2017-05-31T13:00:04Z

@dpmills thanks for your comments, I addressed all of them, PTAL.

asfbot · 2017-05-31T13:31:07Z

Build finished.
--none--

asfbot · 2017-05-31T15:13:46Z

Build finished.
--none--

dpmills · 2017-05-31T17:29:42Z

LGTM

I'm not a committer however, so over to @dhalperi for comitting.

dhalperi · 2017-05-31T17:39:07Z

I don't know how to merge this. Too many commits, too many authors, request not to squash.

This is going to really pollute history if merged as-is, but that is quite possibly by design.

I'll let @iemejia do it, he's a committer :)

dhalperi · 2017-05-31T17:40:27Z

Additionally, I believe there are as-yet-unresolved issues about module names and paths of code.

R: @davorbonaci for some input.

iemejia · 2017-05-31T21:41:50Z

@dhalperi I will see with @echauchot to reduce the number of commits, I think we can still squash some of the latest commits (that addressed the review comments + did the latest fixes before the PR), anyway you have to consider that this is a long ongoing work with three different authors so it is normal to have a reasonable number of commits, I won't consider this pollution because there are not intentionally tiny commits, most of them are like this by design, but we will see still how to reduce them.

iemejia · 2017-05-31T21:47:15Z

For reference @davorbonaci the last point Dan mentions is the paths, currently Nexmark uses the original path that Mark had for the PR: integration/java/nexmark This introduces a new directory structure for integration tests on Beam, this can make sense I think and even be used to have also part of the things @ssisk is working on.
In a brief discussion with Dan we also talked about the possibility of putting this into the examples directory but after discussing with @echauchot we considered that it didn't make sense either because Nexmark is not just a example, it has clearly a different scope.
So in the end we need you to confirm us if you are ok with this new path or to propose us some alternative so we can refactor it if needed.

echauchot · 2017-06-01T09:20:30Z

@dhalperi we managed to reduce the number of commits from 55 to 27: we wanted to keep some big refactorings or some big subjects (like queries fixing/improving) separated. I just pushed the branch. WDYT?

coveralls · 2017-06-02T09:18:44Z

Coverage decreased (-2.1%) to 68.548% when pulling 6232a61 on iemejia:BEAM-160-nexmark into 217f085 on apache:master.

davorbonaci

Very nice! Left a few comments.

davorbonaci · 2017-06-06T20:25:08Z

integration/java/nexmark/BEAM_ON_FLINK_ON_GCP.md

@@ -0,0 +1,282 @@
+<!--


sdks/java/nexmark?

It would be great to have it in the sdk! I am in favor of it. The only question I ask myself is: Nexmark is more a benchmarking suite, not an API or framework. Is it not a problem to have a piece of software that is not used to build software in the sdk module?

davorbonaci · 2017-06-06T20:25:45Z

integration/java/nexmark/BEAM_ON_FLINK_ON_GCP.md

+    under the License.
+-->
+
+# Running NexMark on Beam on Flink on Google Compute Platform


Move to the website; contribution section?

Thanks for pointing that out! Actually it is a leftover, I forgot that file. I propose to simply remove it because we have not tested to run Nexmark that way since Mark did; more important, I prefer communicating about running NexMark on a standard Flink cluster similarly to what is described in the readme about Spark cluster.

davorbonaci · 2017-06-06T20:26:06Z

integration/java/nexmark/README.md

+    under the License.
+-->
+
+# NEXMark integration suite


I guess you mean having this content in both beam website and the readme, right?

Ok, what I propose is to add a new section to the website with a short introduction to Nexmark similar to the talk we gave at the ApacheCon and a reference the Readme (once it is merged) to avoid duplication of information.
Do you find it ok?

davorbonaci · 2017-06-06T20:27:22Z

integration/java/nexmark/pom.xml

+    <relativePath>../pom.xml</relativePath>
+  </parent>
+
+  <artifactId>beam-integration-java-nexmark</artifactId>


beam-sdks-java-nexmark

davorbonaci · 2017-06-06T20:27:37Z

integration/java/nexmark/pom.xml

+
+  <parent>
+    <groupId>org.apache.beam</groupId>
+    <artifactId>beam-integration-java-parent</artifactId>


beam-sdks-java-parent

+1 modulo my interrogation in your first comment

davorbonaci · 2017-06-06T20:35:41Z

integration/java/nexmark/src/main/java/org/apache/beam/integration/nexmark/package-info.java

+/**
+ * Nexmark.
+ */
+package org.apache.beam.integration.nexmark;


Please double-check the output of the aggregated javadoc. Nexmark packages shouldn't be there.

davorbonaci · 2017-06-06T20:37:02Z

integration/java/nexmark/src/main/java/org/apache/beam/integration/nexmark/queries/Query0.java

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.integration.nexmark.queries;


org.apache.beam.sdk.nexmark.XXX (across the board)

davorbonaci · 2017-06-06T20:39:11Z

pom.xml

    <module>sdks</module>
    <module>runners</module>
    <module>examples</module>
+    <module>integration</module>


davorbonaci · 2017-06-06T20:39:19Z

integration/pom.xml

@@ -0,0 +1,51 @@
+<?xml version="1.0" encoding="UTF-8"?>


davorbonaci · 2017-06-06T20:39:32Z

integration/java/pom.xml

@@ -0,0 +1,37 @@
+<?xml version="1.0" encoding="UTF-8"?>


echauchot · 2017-06-07T10:06:13Z

Thanks @davorbonaci for your comments ! I'll be in meetings today, but I'll address them ASAP.

coveralls · 2017-06-15T11:24:24Z

Coverage decreased (-2.2%) to 68.524% when pulling bd0bac5 on iemejia:BEAM-160-nexmark into 217f085 on apache:master.

coveralls · 2017-06-15T16:31:49Z

Coverage decreased (-0.008%) to 70.674% when pulling 45206f6 on iemejia:BEAM-160-nexmark into 217f085 on apache:master.

echauchot · 2017-06-16T08:41:42Z

Davor, thanks for your review, I have addressed your comments, see the last 4 commits. I have also rebased on master. I will squash all the review comments into Fix general issues commit when you find the PR is ok.

coveralls · 2017-06-16T10:01:46Z

Coverage decreased (-0.006%) to 70.67% when pulling 85d743f on iemejia:BEAM-160-nexmark into c528fb2 on apache:master.

coveralls · 2017-06-16T10:40:16Z

Coverage increased (+0.01%) to 70.687% when pulling 85d743f on iemejia:BEAM-160-nexmark into c528fb2 on apache:master.

coveralls · 2017-06-16T11:04:51Z

Coverage increased (+0.02%) to 70.695% when pulling 85d743f on iemejia:BEAM-160-nexmark into c528fb2 on apache:master.

reuvenlax · 2017-07-12T19:40:13Z

Is this PR blocked on something right now?

Update configuration of events generation to add some variation Update execution matrix (issue #45)

echauchot · 2017-08-11T15:00:08Z

@jbonofre moved to 2.2.0-SNAPSHOT and rebased on master

reuvenlax · 2017-08-11T15:17:46Z

@davorbonaci, any more concerns with this PR?

coveralls · 2017-08-11T16:36:20Z

Coverage decreased (-0.01%) to 69.961% when pulling b3d1f81 on iemejia:BEAM-160-nexmark into 07e8cd5 on apache:master.

davorbonaci

LGTM

Sorry for my delay here. At this point, I think this should just be merged. I do think there are open questions such as whether we release this module to Maven Central, does the code belong to the regular jar or test-jar, etc.

Regardless of those questions, please move forward and we'll figure things out, as needed.

echauchot · 2017-08-23T07:26:42Z

I'm very glad to see nexmark move forward, thanks! I'm doing a final rebase of some commits.

coveralls · 2017-08-23T09:11:14Z

Coverage decreased (-0.007%) to 69.974% when pulling 4a6c9f0 on iemejia:BEAM-160-nexmark into 9b175cc on apache:master.

coveralls · 2017-08-23T11:37:04Z

Coverage decreased (-0.01%) to 69.971% when pulling 6d068f7 on iemejia:BEAM-160-nexmark into 9b175cc on apache:master.

coveralls · 2017-08-23T14:45:42Z

Coverage decreased (-0.01%) to 69.967% when pulling 7aa698c on iemejia:BEAM-160-nexmark into 9b175cc on apache:master.

issue #5 and issue #51

…ation code - Use state API in NexmarkUtils.diskBusy() - Remove commented code for direct runner randomization disabling: direct runner no more allows disabling randomization and queries and UT pass

Clean pom, exclude nexmark packages from aggregated javadoc, put spark logs in WARN Update execution matrix in README: Flink termination of streaming pipelines is now ok as far as Nexmark is concerned Remove how to run Nexmark on Flink on Google Cloud Platform from README Update command lines in README after moving nexmark from integration to sdks module

coveralls · 2017-08-23T16:46:23Z

Coverage increased (+0.003%) to 69.977% when pulling d83bd07 on iemejia:BEAM-160-nexmark into f0ce31b on apache:master.

mshields822 · 2017-08-23T18:28:26Z

Thanks for all your great work on this Etienne and Ismaël! Happy to see it closed at last.

Best,
-Mark

iemejia · 2017-08-23T20:22:23Z

No, the big credit is up to you @mshields822 this would not have been possible at all without your work, we just pushed it a bit to the last line, you did the long journey. Thanks for the support and of course for your nice creation.

echauchot · 2017-08-24T06:33:17Z

I could not have said it better :)

dpmills suggested changes May 24, 2017

View reviewed changes

echauchot force-pushed the BEAM-160-nexmark branch from 2c7b50b to 3d0617c Compare May 31, 2017 14:40

echauchot force-pushed the BEAM-160-nexmark branch from 3d0617c to e01fd05 Compare June 1, 2017 09:14

echauchot force-pushed the BEAM-160-nexmark branch from e01fd05 to 6232a61 Compare June 2, 2017 07:55

davorbonaci requested changes Jun 6, 2017

View reviewed changes

echauchot force-pushed the BEAM-160-nexmark branch from 45206f6 to 8f23001 Compare June 16, 2017 08:41

echauchot added 5 commits August 11, 2017 15:56

Clean some code that is specific to Dataflow

0024829

Rename NexmarkDriver to Main and NexmarkRunner to NexmarkLauncher

e19df70

Change benchmark workload settings

564acea

Update configuration of events generation to add some variation Update execution matrix (issue #45)

Remove references to dataflow in generic classes

deec234

Migrate to Beam 2.1.0-SNAPSHOT

50fddde

echauchot force-pushed the BEAM-160-nexmark branch from 85d743f to b3d1f81 Compare August 11, 2017 14:58

davorbonaci approved these changes Aug 22, 2017

View reviewed changes

echauchot force-pushed the BEAM-160-nexmark branch from b3d1f81 to 4a6c9f0 Compare August 23, 2017 07:20

echauchot force-pushed the BEAM-160-nexmark branch from 4a6c9f0 to 6d068f7 Compare August 23, 2017 09:58

echauchot force-pushed the BEAM-160-nexmark branch 2 times, most recently from 75a4ae5 to 7aa698c Compare August 23, 2017 13:11

echauchot added 5 commits August 23, 2017 17:08

Fix query10 log messages

46f7676

issue #5 and issue #51

Improve NexmarkUtils: improve diskBusy() and remove unneeded randomiz…

06ee9a4

…ation code - Use state API in NexmarkUtils.diskBusy() - Remove commented code for direct runner randomization disabling: direct runner no more allows disabling randomization and queries and UT pass

Clean, fix findbugs, fix checkstyle

e659cd2

Move module beam-integration-java-nexmark to beam-sdks-java-nexmark

366de08

echauchot force-pushed the BEAM-160-nexmark branch from 7aa698c to d83bd07 Compare August 23, 2017 15:16

asfgit closed this in 64ff21f Aug 23, 2017

iemejia deleted the BEAM-160-nexmark branch August 24, 2017 09:35

[BEAM-160] Port 'NexMark Queries' to Beam for use as integration test #3114

[BEAM-160] Port 'NexMark Queries' to Beam for use as integration test #3114

Uh oh!

Conversation

echauchot commented May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented May 12, 2017

Uh oh!

jbonofre commented May 12, 2017

Uh oh!

dhalperi commented May 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented May 30, 2017

Uh oh!

asfbot commented May 30, 2017

Uh oh!

asfbot commented May 31, 2017

Uh oh!

echauchot commented May 31, 2017

Uh oh!

asfbot commented May 31, 2017

Uh oh!

asfbot commented May 31, 2017

Uh oh!

dpmills commented May 31, 2017

Uh oh!

dhalperi commented May 31, 2017

Uh oh!

dhalperi commented May 31, 2017

Uh oh!

iemejia commented May 31, 2017

Uh oh!

iemejia commented May 31, 2017

Uh oh!

echauchot commented Jun 1, 2017

Uh oh!

coveralls commented Jun 2, 2017

Uh oh!

davorbonaci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

echauchot commented May 12, 2017 •

edited

Loading

echauchot Jun 15, 2017 •

edited

Loading