ibzib commented on a change in pull request #13743:
URL: https://github.com/apache/beam/pull/13743#discussion_r568825987



##########
File path: 
runners/spark/src/main/java/org/apache/beam/runners/spark/SparkPipelineRunner.java
##########
@@ -212,6 +247,22 @@ public PortablePipelineResult run(RunnerApi.Pipeline 
pipeline, JobInfo jobInfo)
             pipelineOptions.as(MetricsOptions.class),
             result);
     metricsPusher.start();
+    if (pipelineOptions.getEventLogEnabled()) {
+      eventLoggingListener.onApplicationStart(
+          new SparkListenerApplicationStart(
+              jsc.getConf().getAppId(),

Review comment:
       This should be appName, not appId. 
https://spark.apache.org/docs/2.4.7/api/java/org/apache/spark/scheduler/SparkListenerApplicationStart.html#SparkListenerApplicationStart-java.lang.String-scala.Option-long-java.lang.String-scala.Option-scala.Option-
   
   ```suggestion
                 pipelineOptions.as(ApplicationNameOptions.class).getAppName(),
   ```

##########
File path: 
runners/spark/src/main/java/org/apache/beam/runners/spark/SparkPipelineRunner.java
##########
@@ -123,10 +134,34 @@ public PortablePipelineResult run(RunnerApi.Pipeline 
pipeline, JobInfo jobInfo)
         "Will stage {} files. (Enable logging at DEBUG level to see which 
files will be staged.)",
         pipelineOptions.getFilesToStage().size());
     LOG.debug("Staging files: {}", pipelineOptions.getFilesToStage());
-
     PortablePipelineResult result;
     final JavaSparkContext jsc = 
SparkContextFactory.getSparkContext(pipelineOptions);
 
+    long startTime = Instant.now().getMillis();
+    EventLoggingListener eventLoggingListener = null;
+    if (pipelineOptions.getEventLogEnabled()) {
+      eventLoggingListener =
+          new EventLoggingListener(
+              jsc.getConf().getAppId(),
+              scala.Option.apply(jobInfo.jobName()),

Review comment:
       appAttemptId is a Spark concept. jobInfo.jobName is a Beam concept, 
which is unrelated.
   
   
https://github.com/apache/spark/blob/d308794adb821d301847772de3ee1ef3166aaf5b/core/src/main/scala/org/apache/spark/scheduler/EventLoggingListener.scala#L60
   
   ```suggestion
                 scala.Option.apply("1"),
   ```

##########
File path: 
runners/spark/src/main/java/org/apache/beam/runners/spark/SparkJobServerDriver.java
##########
@@ -36,6 +36,7 @@
 
   /** Spark runner-specific Configuration for the jobServer. */
   public static class SparkServerConfiguration extends ServerConfiguration {
+

Review comment:
       Nit: remove whitespace changes to this file.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to