Will-Lo commented on code in PR #3894:
URL: https://github.com/apache/gobblin/pull/3894#discussion_r1522153143
##########
gobblin-temporal/src/main/java/org/apache/gobblin/temporal/ddm/launcher/ProcessWorkUnitsJobLauncher.java:
##########
@@ -85,7 +85,9 @@ public void submitJob(List<WorkUnit> workunits) {
URI nameNodeUri = new URI(PropertiesUtils.getRequiredProp(this.jobProps,
GOBBLIN_TEMPORAL_JOB_LAUNCHER_ARG_NAME_NODE_URI));
// NOTE: `Path` is challenging for temporal to ser/de, but nonetheless
do pre-construct as `Path`, to pre-validate this prop string's contents
Path workUnitsDir = new
Path(PropertiesUtils.getRequiredProp(this.jobProps,
GOBBLIN_TEMPORAL_JOB_LAUNCHER_ARG_WORK_UNITS_DIR));
- EventSubmitterContext eventSubmitterContext = new
EventSubmitterContext(this.eventSubmitter);
+ EventSubmitterContext eventSubmitterContext = new
EventSubmitterContext.Builder()
+ .withEventSubmitter(eventSubmitter)
+ .build();
Review Comment:
There's multiple ways of initializing the event submitter context, either
via providing an event submitter or by manually specifying the tags or with the
job properties, so I'd rather have it go through the builder.
Also since the metric tags are created using an immutable list in Gobblin it
becomes difficult to append to, which is why the builder uses an array list and
then creates an immutable list when the tags have been initialized. If the
previous tags were immutable then it's impossible to keep the field for tags
before as `final` when more tags need to be added like what was mentioned in
the previous comments.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
