XComp commented on a change in pull request #13311:
URL: https://github.com/apache/flink/pull/13311#discussion_r488447535
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/util/config/memory/CommonProcessMemorySpec.java
##########
@@ -101,7 +101,7 @@ public MemorySize getTotalProcessMemorySize() {
public boolean equals(Object obj) {
if (obj == this) {
return true;
- } else if (obj instanceof CommonProcessMemorySpec<?> ) {
+ } else if (getClass().equals(obj.getClass()) && obj instanceof
CommonProcessMemorySpec<?> ) {
Review comment:
Comparing the classes of both objects here make the `instanceof` call
obsolete. You either go for `getClass()` or `instanceof` considering both
having slightly different semantics but both having the same outcome if
`getClass().equals(obj.getClass())` applies.
I did some further research in that matter: I haven't found any evidence
that `JobManagerProcessSpec` and `TaskManagerProcessSpec` are ever used in the
same context. This makes the case of `equals` being non-symmetric when dealing
with subclasses kind of theoretical. But if you want to reduce the risk of this
coming up later on I see two possible approaches:
1. Check the equality of both instance's classes (Keep in mind to add a
`null` check for `obj` in that case).
2. You move the `equals(Object)` implementation into `JobProcessMemorySpec`
and make it `final`. In that case, you can keep using `instanceof`.
----------------------------------------------------------------
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]
