XComp commented on a change in pull request #14804:
URL: https://github.com/apache/flink/pull/14804#discussion_r580067125
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/jobmaster/JobManagerRunnerResult.java
##########
@@ -81,21 +83,23 @@ public boolean equals(Object o) {
return false;
}
JobManagerRunnerResult that = (JobManagerRunnerResult) o;
- return Objects.equals(archivedExecutionGraph,
that.archivedExecutionGraph)
+ // TODO: verify ExecutionGraphInfo.equals
Review comment:
I wanted to re-iterate over the `equals`/`hashCode` contract for
`ExecutionGraphInfo` and `ArchivedExecutionGraph`. The `JobManagerRunnerResult`
must have slipped through when going through the `TODO`s.
I was just wondering whether there should have been an `equals`/`hashCode`
implementation for `ArchivedExecutionGraph` in the first place. Isn't
`ArchivedExecutionGraph` well-defined by its `ArchivedExecutionJobVertex`
instances stored in `ArchivedExecutionGraph::tasks`? And
`ArchivedExecutionJobVertex` would be well-defined through its
`ArchivedExecutionVertex` instances stored in
`ArchivedExecutionJobVertex::taskVertices` which would be defined through its
`ArchivedExecutionVertex::currentExecution`? I guess, we wouldn't gain anything
code-wise besides having the contract formulated in the code which might
increase readability. Any thoughts on this @zentol @tillrohrmann ?
----------------------------------------------------------------
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:
us...@infra.apache.org
