phet commented on code in PR #3853:
URL: https://github.com/apache/gobblin/pull/3853#discussion_r1444999117
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/flowgraph/Dag.java:
##########
@@ -255,10 +261,14 @@ public static class DagNode<T> {
private T value;
//List of parent Nodes that are dependencies of this Node.
private List<DagNode<T>> parentNodes;
+ private String id;
//Constructor
public DagNode(T value) {
this.value = value;
+ if (this.getValue() instanceof JobExecutionPlan) {
+ this.id = createId(((JobExecutionPlan)
this.getValue()).getJobSpec().getConfig());
+ }
Review Comment:
looks like you're allowing this to default to `null`-initialized when not
`instanceof JobExecutionPlan`.
given that is not even this instance's type, but that of the generic param
it encloses, it suggests this not to be essential to the abstraction. as such
it wouldn't belong as a field (property) of class instances.
alternatively it might live self-contained, calculated each time within a
`static` (e.g. in `DagManagerUtils`):
```
public static Optional<String> extractJobId(DagNode<T>)
```
less good might be in a stand-alone accessor (not burying conditional code
within the ctor):
```
public Optional<String> getJobId()
```
--
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]
