sv2000 commented on a change in pull request #3076:
URL: https://github.com/apache/incubator-gobblin/pull/3076#discussion_r487317913
##########
File path:
gobblin-runtime/src/main/java/org/apache/gobblin/service/monitoring/JobStatus.java
##########
@@ -17,10 +17,11 @@
package org.apache.gobblin.service.monitoring;
-import org.apache.gobblin.annotation.Alpha;
-
import lombok.Builder;
import lombok.Getter;
+import lombok.Setter;
+
+import org.apache.gobblin.annotation.Alpha;
Review comment:
We can remove this annotation.
##########
File path:
gobblin-service/src/main/java/org/apache/gobblin/service/modules/spec/JobExecutionPlan.java
##########
@@ -82,7 +87,8 @@ public JobExecutionPlan createPlan(FlowSpec flowSpec, Config
jobConfig, SpecExec
* @param flowSpec input FlowSpec.
* @return a {@link JobSpec} corresponding to the resolved job config.
*/
- private static JobSpec buildJobSpec(FlowSpec flowSpec, Config jobConfig,
Long flowExecutionId, Config sysConfig) throws URISyntaxException {
+ private static JobSpec buildJobSpec(FlowSpec flowSpec, Config jobConfig,
Long flowExecutionId, Config sysConfig, Config specExecutorConfig)
Review comment:
Modify the javadoc to reflect the current list of arguments.
##########
File path:
gobblin-service/src/main/java/org/apache/gobblin/service/modules/spec/JobExecutionPlan.java
##########
@@ -154,7 +160,7 @@ private static JobSpec buildJobSpec(FlowSpec flowSpec,
Config jobConfig, Long fl
* execution status of the job.
* @param jobSpec representing a fully resolved {@link JobSpec}.
*/
- private static void addAdditionalConfig(JobSpec jobSpec, Config sysConfig)
{
+ private static void addAdditionalConfig(JobSpec jobSpec, Config sysConfig,
Config specExecutorConfig) {
Review comment:
Modify the javadoc.
##########
File path:
gobblin-metrics-libs/gobblin-metrics-base/src/main/java/org/apache/gobblin/metrics/MetricNames.java
##########
@@ -22,6 +22,8 @@
*/
public class MetricNames {
+ public static final String NUM_WORKUNITS = "numWorkUnits";
Review comment:
Should this be grouped into SourceMetrics and have the metric name as
"gobblin.source.numWorkUnits.created" to be consistent with other metric names?
##########
File path:
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/AbstractJobLauncher.java
##########
@@ -414,6 +415,9 @@ public void apply(JobListener jobListener, JobContext
jobContext)
workUnitsCreationTimer.stop(this.eventMetadataGenerator.getMetadata(this.jobContext,
EventName.WORK_UNITS_CREATION));
+ this.jobContext.getJobState().setProp(
+ MetricNames.NUM_WORKUNITS,
workUnitStream.getMaterializedWorkUnitCollection().size());
Review comment:
On second reading - Is numWorkunits a CodaHale metric that gets emitted
via MetricReporter? I think the metrics listed in MetricNames are intended for
the reporter metrics. If numWorkUnits is intended for a different purpose,
let's not add it into MetricNames class.
##########
File path:
gobblin-runtime/src/main/java/org/apache/gobblin/service/monitoring/JobStatus.java
##########
@@ -17,10 +17,11 @@
package org.apache.gobblin.service.monitoring;
-import org.apache.gobblin.annotation.Alpha;
-
import lombok.Builder;
import lombok.Getter;
+import lombok.Setter;
+
+import org.apache.gobblin.annotation.Alpha;
Review comment:
We can remove this annotation.
##########
File path:
gobblin-service/src/main/java/org/apache/gobblin/service/modules/spec/JobExecutionPlan.java
##########
@@ -82,7 +87,8 @@ public JobExecutionPlan createPlan(FlowSpec flowSpec, Config
jobConfig, SpecExec
* @param flowSpec input FlowSpec.
* @return a {@link JobSpec} corresponding to the resolved job config.
*/
- private static JobSpec buildJobSpec(FlowSpec flowSpec, Config jobConfig,
Long flowExecutionId, Config sysConfig) throws URISyntaxException {
+ private static JobSpec buildJobSpec(FlowSpec flowSpec, Config jobConfig,
Long flowExecutionId, Config sysConfig, Config specExecutorConfig)
Review comment:
Modify the javadoc to reflect the current list of arguments.
##########
File path:
gobblin-service/src/main/java/org/apache/gobblin/service/modules/spec/JobExecutionPlan.java
##########
@@ -154,7 +160,7 @@ private static JobSpec buildJobSpec(FlowSpec flowSpec,
Config jobConfig, Long fl
* execution status of the job.
* @param jobSpec representing a fully resolved {@link JobSpec}.
*/
- private static void addAdditionalConfig(JobSpec jobSpec, Config sysConfig)
{
+ private static void addAdditionalConfig(JobSpec jobSpec, Config sysConfig,
Config specExecutorConfig) {
Review comment:
Modify the javadoc.
##########
File path:
gobblin-metrics-libs/gobblin-metrics-base/src/main/java/org/apache/gobblin/metrics/MetricNames.java
##########
@@ -22,6 +22,8 @@
*/
public class MetricNames {
+ public static final String NUM_WORKUNITS = "numWorkUnits";
Review comment:
Should this be grouped into SourceMetrics and have the metric name as
"gobblin.source.numWorkUnits.created" to be consistent with other metric names?
##########
File path:
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/AbstractJobLauncher.java
##########
@@ -414,6 +415,9 @@ public void apply(JobListener jobListener, JobContext
jobContext)
workUnitsCreationTimer.stop(this.eventMetadataGenerator.getMetadata(this.jobContext,
EventName.WORK_UNITS_CREATION));
+ this.jobContext.getJobState().setProp(
+ MetricNames.NUM_WORKUNITS,
workUnitStream.getMaterializedWorkUnitCollection().size());
Review comment:
On second reading - Is numWorkunits a CodaHale metric that gets emitted
via MetricReporter? I think the metrics listed in MetricNames are intended for
the reporter metrics. If numWorkUnits is intended for a different purpose,
let's not add it into MetricNames class.
##########
File path:
gobblin-runtime/src/main/java/org/apache/gobblin/service/monitoring/JobStatus.java
##########
@@ -17,10 +17,11 @@
package org.apache.gobblin.service.monitoring;
-import org.apache.gobblin.annotation.Alpha;
-
import lombok.Builder;
import lombok.Getter;
+import lombok.Setter;
+
+import org.apache.gobblin.annotation.Alpha;
Review comment:
We can remove this annotation.
##########
File path:
gobblin-service/src/main/java/org/apache/gobblin/service/modules/spec/JobExecutionPlan.java
##########
@@ -82,7 +87,8 @@ public JobExecutionPlan createPlan(FlowSpec flowSpec, Config
jobConfig, SpecExec
* @param flowSpec input FlowSpec.
* @return a {@link JobSpec} corresponding to the resolved job config.
*/
- private static JobSpec buildJobSpec(FlowSpec flowSpec, Config jobConfig,
Long flowExecutionId, Config sysConfig) throws URISyntaxException {
+ private static JobSpec buildJobSpec(FlowSpec flowSpec, Config jobConfig,
Long flowExecutionId, Config sysConfig, Config specExecutorConfig)
Review comment:
Modify the javadoc to reflect the current list of arguments.
##########
File path:
gobblin-service/src/main/java/org/apache/gobblin/service/modules/spec/JobExecutionPlan.java
##########
@@ -154,7 +160,7 @@ private static JobSpec buildJobSpec(FlowSpec flowSpec,
Config jobConfig, Long fl
* execution status of the job.
* @param jobSpec representing a fully resolved {@link JobSpec}.
*/
- private static void addAdditionalConfig(JobSpec jobSpec, Config sysConfig)
{
+ private static void addAdditionalConfig(JobSpec jobSpec, Config sysConfig,
Config specExecutorConfig) {
Review comment:
Modify the javadoc.
##########
File path:
gobblin-metrics-libs/gobblin-metrics-base/src/main/java/org/apache/gobblin/metrics/MetricNames.java
##########
@@ -22,6 +22,8 @@
*/
public class MetricNames {
+ public static final String NUM_WORKUNITS = "numWorkUnits";
Review comment:
Should this be grouped into SourceMetrics and have the metric name as
"gobblin.source.numWorkUnits.created" to be consistent with other metric names?
##########
File path:
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/AbstractJobLauncher.java
##########
@@ -414,6 +415,9 @@ public void apply(JobListener jobListener, JobContext
jobContext)
workUnitsCreationTimer.stop(this.eventMetadataGenerator.getMetadata(this.jobContext,
EventName.WORK_UNITS_CREATION));
+ this.jobContext.getJobState().setProp(
+ MetricNames.NUM_WORKUNITS,
workUnitStream.getMaterializedWorkUnitCollection().size());
Review comment:
On second reading - Is numWorkunits a CodaHale metric that gets emitted
via MetricReporter? I think the metrics listed in MetricNames are intended for
the reporter metrics. If numWorkUnits is intended for a different purpose,
let's not add it into MetricNames class.
##########
File path:
gobblin-runtime/src/main/java/org/apache/gobblin/service/monitoring/JobStatus.java
##########
@@ -17,10 +17,11 @@
package org.apache.gobblin.service.monitoring;
-import org.apache.gobblin.annotation.Alpha;
-
import lombok.Builder;
import lombok.Getter;
+import lombok.Setter;
+
+import org.apache.gobblin.annotation.Alpha;
Review comment:
We can remove this annotation.
##########
File path:
gobblin-service/src/main/java/org/apache/gobblin/service/modules/spec/JobExecutionPlan.java
##########
@@ -82,7 +87,8 @@ public JobExecutionPlan createPlan(FlowSpec flowSpec, Config
jobConfig, SpecExec
* @param flowSpec input FlowSpec.
* @return a {@link JobSpec} corresponding to the resolved job config.
*/
- private static JobSpec buildJobSpec(FlowSpec flowSpec, Config jobConfig,
Long flowExecutionId, Config sysConfig) throws URISyntaxException {
+ private static JobSpec buildJobSpec(FlowSpec flowSpec, Config jobConfig,
Long flowExecutionId, Config sysConfig, Config specExecutorConfig)
Review comment:
Modify the javadoc to reflect the current list of arguments.
##########
File path:
gobblin-service/src/main/java/org/apache/gobblin/service/modules/spec/JobExecutionPlan.java
##########
@@ -154,7 +160,7 @@ private static JobSpec buildJobSpec(FlowSpec flowSpec,
Config jobConfig, Long fl
* execution status of the job.
* @param jobSpec representing a fully resolved {@link JobSpec}.
*/
- private static void addAdditionalConfig(JobSpec jobSpec, Config sysConfig)
{
+ private static void addAdditionalConfig(JobSpec jobSpec, Config sysConfig,
Config specExecutorConfig) {
Review comment:
Modify the javadoc.
##########
File path:
gobblin-metrics-libs/gobblin-metrics-base/src/main/java/org/apache/gobblin/metrics/MetricNames.java
##########
@@ -22,6 +22,8 @@
*/
public class MetricNames {
+ public static final String NUM_WORKUNITS = "numWorkUnits";
Review comment:
Should this be grouped into SourceMetrics and have the metric name as
"gobblin.source.numWorkUnits.created" to be consistent with other metric names?
##########
File path:
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/AbstractJobLauncher.java
##########
@@ -414,6 +415,9 @@ public void apply(JobListener jobListener, JobContext
jobContext)
workUnitsCreationTimer.stop(this.eventMetadataGenerator.getMetadata(this.jobContext,
EventName.WORK_UNITS_CREATION));
+ this.jobContext.getJobState().setProp(
+ MetricNames.NUM_WORKUNITS,
workUnitStream.getMaterializedWorkUnitCollection().size());
Review comment:
On second reading - Is numWorkunits a CodaHale metric that gets emitted
via MetricReporter? I think the metrics listed in MetricNames are intended for
the reporter metrics. If numWorkUnits is intended for a different purpose,
let's not add it into MetricNames class.
##########
File path:
gobblin-runtime/src/main/java/org/apache/gobblin/service/monitoring/JobStatus.java
##########
@@ -17,10 +17,11 @@
package org.apache.gobblin.service.monitoring;
-import org.apache.gobblin.annotation.Alpha;
-
import lombok.Builder;
import lombok.Getter;
+import lombok.Setter;
+
+import org.apache.gobblin.annotation.Alpha;
Review comment:
We can remove this annotation.
##########
File path:
gobblin-service/src/main/java/org/apache/gobblin/service/modules/spec/JobExecutionPlan.java
##########
@@ -82,7 +87,8 @@ public JobExecutionPlan createPlan(FlowSpec flowSpec, Config
jobConfig, SpecExec
* @param flowSpec input FlowSpec.
* @return a {@link JobSpec} corresponding to the resolved job config.
*/
- private static JobSpec buildJobSpec(FlowSpec flowSpec, Config jobConfig,
Long flowExecutionId, Config sysConfig) throws URISyntaxException {
+ private static JobSpec buildJobSpec(FlowSpec flowSpec, Config jobConfig,
Long flowExecutionId, Config sysConfig, Config specExecutorConfig)
Review comment:
Modify the javadoc to reflect the current list of arguments.
##########
File path:
gobblin-service/src/main/java/org/apache/gobblin/service/modules/spec/JobExecutionPlan.java
##########
@@ -154,7 +160,7 @@ private static JobSpec buildJobSpec(FlowSpec flowSpec,
Config jobConfig, Long fl
* execution status of the job.
* @param jobSpec representing a fully resolved {@link JobSpec}.
*/
- private static void addAdditionalConfig(JobSpec jobSpec, Config sysConfig)
{
+ private static void addAdditionalConfig(JobSpec jobSpec, Config sysConfig,
Config specExecutorConfig) {
Review comment:
Modify the javadoc.
##########
File path:
gobblin-metrics-libs/gobblin-metrics-base/src/main/java/org/apache/gobblin/metrics/MetricNames.java
##########
@@ -22,6 +22,8 @@
*/
public class MetricNames {
+ public static final String NUM_WORKUNITS = "numWorkUnits";
Review comment:
Should this be grouped into SourceMetrics and have the metric name as
"gobblin.source.numWorkUnits.created" to be consistent with other metric names?
##########
File path:
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/AbstractJobLauncher.java
##########
@@ -414,6 +415,9 @@ public void apply(JobListener jobListener, JobContext
jobContext)
workUnitsCreationTimer.stop(this.eventMetadataGenerator.getMetadata(this.jobContext,
EventName.WORK_UNITS_CREATION));
+ this.jobContext.getJobState().setProp(
+ MetricNames.NUM_WORKUNITS,
workUnitStream.getMaterializedWorkUnitCollection().size());
Review comment:
On second reading - Is numWorkunits a CodaHale metric that gets emitted
via MetricReporter? I think the metrics listed in MetricNames are intended for
the reporter metrics. If numWorkUnits is intended for a different purpose,
let's not add it into MetricNames class.
##########
File path:
gobblin-runtime/src/main/java/org/apache/gobblin/service/monitoring/JobStatus.java
##########
@@ -17,10 +17,11 @@
package org.apache.gobblin.service.monitoring;
-import org.apache.gobblin.annotation.Alpha;
-
import lombok.Builder;
import lombok.Getter;
+import lombok.Setter;
+
+import org.apache.gobblin.annotation.Alpha;
Review comment:
We can remove this annotation.
##########
File path:
gobblin-service/src/main/java/org/apache/gobblin/service/modules/spec/JobExecutionPlan.java
##########
@@ -82,7 +87,8 @@ public JobExecutionPlan createPlan(FlowSpec flowSpec, Config
jobConfig, SpecExec
* @param flowSpec input FlowSpec.
* @return a {@link JobSpec} corresponding to the resolved job config.
*/
- private static JobSpec buildJobSpec(FlowSpec flowSpec, Config jobConfig,
Long flowExecutionId, Config sysConfig) throws URISyntaxException {
+ private static JobSpec buildJobSpec(FlowSpec flowSpec, Config jobConfig,
Long flowExecutionId, Config sysConfig, Config specExecutorConfig)
Review comment:
Modify the javadoc to reflect the current list of arguments.
##########
File path:
gobblin-service/src/main/java/org/apache/gobblin/service/modules/spec/JobExecutionPlan.java
##########
@@ -154,7 +160,7 @@ private static JobSpec buildJobSpec(FlowSpec flowSpec,
Config jobConfig, Long fl
* execution status of the job.
* @param jobSpec representing a fully resolved {@link JobSpec}.
*/
- private static void addAdditionalConfig(JobSpec jobSpec, Config sysConfig)
{
+ private static void addAdditionalConfig(JobSpec jobSpec, Config sysConfig,
Config specExecutorConfig) {
Review comment:
Modify the javadoc.
##########
File path:
gobblin-metrics-libs/gobblin-metrics-base/src/main/java/org/apache/gobblin/metrics/MetricNames.java
##########
@@ -22,6 +22,8 @@
*/
public class MetricNames {
+ public static final String NUM_WORKUNITS = "numWorkUnits";
Review comment:
Should this be grouped into SourceMetrics and have the metric name as
"gobblin.source.numWorkUnits.created" to be consistent with other metric names?
##########
File path:
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/AbstractJobLauncher.java
##########
@@ -414,6 +415,9 @@ public void apply(JobListener jobListener, JobContext
jobContext)
workUnitsCreationTimer.stop(this.eventMetadataGenerator.getMetadata(this.jobContext,
EventName.WORK_UNITS_CREATION));
+ this.jobContext.getJobState().setProp(
+ MetricNames.NUM_WORKUNITS,
workUnitStream.getMaterializedWorkUnitCollection().size());
Review comment:
On second reading - Is numWorkunits a CodaHale metric that gets emitted
via MetricReporter? I think the metrics listed in MetricNames are intended for
the reporter metrics. If numWorkUnits is intended for a different purpose,
let's not add it into MetricNames class.
##########
File path:
gobblin-runtime/src/main/java/org/apache/gobblin/service/monitoring/JobStatus.java
##########
@@ -17,10 +17,11 @@
package org.apache.gobblin.service.monitoring;
-import org.apache.gobblin.annotation.Alpha;
-
import lombok.Builder;
import lombok.Getter;
+import lombok.Setter;
+
+import org.apache.gobblin.annotation.Alpha;
Review comment:
We can remove this annotation.
##########
File path:
gobblin-service/src/main/java/org/apache/gobblin/service/modules/spec/JobExecutionPlan.java
##########
@@ -82,7 +87,8 @@ public JobExecutionPlan createPlan(FlowSpec flowSpec, Config
jobConfig, SpecExec
* @param flowSpec input FlowSpec.
* @return a {@link JobSpec} corresponding to the resolved job config.
*/
- private static JobSpec buildJobSpec(FlowSpec flowSpec, Config jobConfig,
Long flowExecutionId, Config sysConfig) throws URISyntaxException {
+ private static JobSpec buildJobSpec(FlowSpec flowSpec, Config jobConfig,
Long flowExecutionId, Config sysConfig, Config specExecutorConfig)
Review comment:
Modify the javadoc to reflect the current list of arguments.
##########
File path:
gobblin-service/src/main/java/org/apache/gobblin/service/modules/spec/JobExecutionPlan.java
##########
@@ -154,7 +160,7 @@ private static JobSpec buildJobSpec(FlowSpec flowSpec,
Config jobConfig, Long fl
* execution status of the job.
* @param jobSpec representing a fully resolved {@link JobSpec}.
*/
- private static void addAdditionalConfig(JobSpec jobSpec, Config sysConfig)
{
+ private static void addAdditionalConfig(JobSpec jobSpec, Config sysConfig,
Config specExecutorConfig) {
Review comment:
Modify the javadoc.
##########
File path:
gobblin-metrics-libs/gobblin-metrics-base/src/main/java/org/apache/gobblin/metrics/MetricNames.java
##########
@@ -22,6 +22,8 @@
*/
public class MetricNames {
+ public static final String NUM_WORKUNITS = "numWorkUnits";
Review comment:
Should this be grouped into SourceMetrics and have the metric name as
"gobblin.source.numWorkUnits.created" to be consistent with other metric names?
##########
File path:
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/AbstractJobLauncher.java
##########
@@ -414,6 +415,9 @@ public void apply(JobListener jobListener, JobContext
jobContext)
workUnitsCreationTimer.stop(this.eventMetadataGenerator.getMetadata(this.jobContext,
EventName.WORK_UNITS_CREATION));
+ this.jobContext.getJobState().setProp(
+ MetricNames.NUM_WORKUNITS,
workUnitStream.getMaterializedWorkUnitCollection().size());
Review comment:
On second reading - Is numWorkunits a CodaHale metric that gets emitted
via MetricReporter? I think the metrics listed in MetricNames are intended for
the reporter metrics. If numWorkUnits is intended for a different purpose,
let's not add it into MetricNames class.
##########
File path:
gobblin-runtime/src/main/java/org/apache/gobblin/service/monitoring/JobStatus.java
##########
@@ -17,10 +17,11 @@
package org.apache.gobblin.service.monitoring;
-import org.apache.gobblin.annotation.Alpha;
-
import lombok.Builder;
import lombok.Getter;
+import lombok.Setter;
+
+import org.apache.gobblin.annotation.Alpha;
Review comment:
We can remove this annotation.
##########
File path:
gobblin-service/src/main/java/org/apache/gobblin/service/modules/spec/JobExecutionPlan.java
##########
@@ -82,7 +87,8 @@ public JobExecutionPlan createPlan(FlowSpec flowSpec, Config
jobConfig, SpecExec
* @param flowSpec input FlowSpec.
* @return a {@link JobSpec} corresponding to the resolved job config.
*/
- private static JobSpec buildJobSpec(FlowSpec flowSpec, Config jobConfig,
Long flowExecutionId, Config sysConfig) throws URISyntaxException {
+ private static JobSpec buildJobSpec(FlowSpec flowSpec, Config jobConfig,
Long flowExecutionId, Config sysConfig, Config specExecutorConfig)
Review comment:
Modify the javadoc to reflect the current list of arguments.
##########
File path:
gobblin-service/src/main/java/org/apache/gobblin/service/modules/spec/JobExecutionPlan.java
##########
@@ -154,7 +160,7 @@ private static JobSpec buildJobSpec(FlowSpec flowSpec,
Config jobConfig, Long fl
* execution status of the job.
* @param jobSpec representing a fully resolved {@link JobSpec}.
*/
- private static void addAdditionalConfig(JobSpec jobSpec, Config sysConfig)
{
+ private static void addAdditionalConfig(JobSpec jobSpec, Config sysConfig,
Config specExecutorConfig) {
Review comment:
Modify the javadoc.
##########
File path:
gobblin-metrics-libs/gobblin-metrics-base/src/main/java/org/apache/gobblin/metrics/MetricNames.java
##########
@@ -22,6 +22,8 @@
*/
public class MetricNames {
+ public static final String NUM_WORKUNITS = "numWorkUnits";
Review comment:
Should this be grouped into SourceMetrics and have the metric name as
"gobblin.source.numWorkUnits.created" to be consistent with other metric names?
##########
File path:
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/AbstractJobLauncher.java
##########
@@ -414,6 +415,9 @@ public void apply(JobListener jobListener, JobContext
jobContext)
workUnitsCreationTimer.stop(this.eventMetadataGenerator.getMetadata(this.jobContext,
EventName.WORK_UNITS_CREATION));
+ this.jobContext.getJobState().setProp(
+ MetricNames.NUM_WORKUNITS,
workUnitStream.getMaterializedWorkUnitCollection().size());
Review comment:
On second reading - Is numWorkunits a CodaHale metric that gets emitted
via MetricReporter? I think the metrics listed in MetricNames are intended for
the reporter metrics. If numWorkUnits is intended for a different purpose,
let's not add it into MetricNames class.
----------------------------------------------------------------
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]
