You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by zhangminglei <gi...@git.apache.org> on 2017/07/14 00:51:38 UTC
[GitHub] flink pull request #4328: [FLINK-6493] Fix ineffective null check in Registe...
GitHub user zhangminglei opened a pull request:
https://github.com/apache/flink/pull/4328
[FLINK-6493] Fix ineffective null check in RegisteredOperatorBackendS…
This PR is simlar to https://github.com/apache/flink/pull/1871/files. @tedyu What do you think of this Change ?
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zhangminglei/flink flink-6493-Ineffective-null-check-in-RegisteredOperatorBackendStateMetaInfo#equals
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/4328.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #4328
----
commit 5e771d6a877867cf450fd7ce40c069db6ce97482
Author: zhangminglei <zm...@163.com>
Date: 2017-07-14T00:48:51Z
[FLINK-6493] Fix ineffective null check in RegisteredOperatorBackendStateMetaInfo#equals
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4328: [FLINK-6493] Fix ineffective null check in Registe...
Posted by tedyu <gi...@git.apache.org>.
Github user tedyu commented on a diff in the pull request:
https://github.com/apache/flink/pull/4328#discussion_r130235370
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/state/RegisteredOperatorBackendStateMetaInfo.java ---
@@ -175,14 +175,21 @@ public boolean equals(Object obj) {
return false;
}
+ Snapshot snapshot;
+
+ if (obj instanceof Snapshot) {
+ snapshot = (Snapshot)obj;
+ } else {
--- End diff --
nit: if you place the condition for else first, declaration and assignment to snapshot can be done in one line.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4328: [FLINK-6493] Fix ineffective null check in RegisteredOper...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei commented on the issue:
https://github.com/apache/flink/pull/4328
Hello, @tzulitai Code have been updated. Please check it out. :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4328: [FLINK-6493] Fix ineffective null check in Registe...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on a diff in the pull request:
https://github.com/apache/flink/pull/4328#discussion_r131107598
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/state/RegisteredOperatorBackendStateMetaInfo.java ---
@@ -175,14 +176,19 @@ public boolean equals(Object obj) {
return false;
}
+ if (!(obj instanceof Snapshot)) {
+ return false;
+ }
+
+ Snapshot snapshot = (Snapshot)obj;
+
// need to check for nulls because serializer and config snapshots may be null on restore
- return (obj instanceof Snapshot)
- && name.equals(((Snapshot) obj).getName())
- && assignmentMode.equals(((Snapshot) obj).getAssignmentMode())
- && ((partitionStateSerializer == null && ((Snapshot) obj).getPartitionStateSerializer() == null)
- || partitionStateSerializer.equals(((Snapshot) obj).getPartitionStateSerializer()))
- && ((partitionStateSerializerConfigSnapshot == null && ((Snapshot) obj).getPartitionStateSerializerConfigSnapshot() == null)
- || partitionStateSerializerConfigSnapshot.equals(((Snapshot) obj).getPartitionStateSerializerConfigSnapshot()));
+ return Objects.equals(name, snapshot.getName())
+ && Objects.equals(assignmentMode, snapshot.getAssignmentMode())
+ && ((partitionStateSerializer == null && (snapshot.getPartitionStateSerializer() == null)
+ || (Objects.equals(partitionStateSerializer, snapshot.getPartitionStateSerializer())))
+ && ((partitionStateSerializerConfigSnapshot == null && (snapshot.getPartitionStateSerializerConfigSnapshot() == null)
+ || (Objects.equals(partitionStateSerializerConfigSnapshot, snapshot.getPartitionStateSerializerConfigSnapshot())))));
--- End diff --
Same for these two.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4328: [FLINK-6493] Fix ineffective null check in RegisteredOper...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/4328
@zhangminglei there seems to be a checkstyle violation. Please check the Travis logs.
Other than that this looks good to merge. Will merge after the violation is fixed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4328: [FLINK-6493] Fix ineffective null check in RegisteredOper...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei commented on the issue:
https://github.com/apache/flink/pull/4328
Thanks to @tedyu . It is a good idea and code have been updated based on your suggestion. Please check it out again ~
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4328: [FLINK-6493] Fix ineffective null check in Registe...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on a diff in the pull request:
https://github.com/apache/flink/pull/4328#discussion_r130831063
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/state/RegisteredOperatorBackendStateMetaInfo.java ---
@@ -175,14 +175,19 @@ public boolean equals(Object obj) {
return false;
}
+ if (!(obj instanceof Snapshot)) {
+ return false;
+ }
+
+ Snapshot snapshot = (Snapshot)obj;
+
// need to check for nulls because serializer and config snapshots may be null on restore
- return (obj instanceof Snapshot)
- && name.equals(((Snapshot) obj).getName())
- && assignmentMode.equals(((Snapshot) obj).getAssignmentMode())
- && ((partitionStateSerializer == null && ((Snapshot) obj).getPartitionStateSerializer() == null)
- || partitionStateSerializer.equals(((Snapshot) obj).getPartitionStateSerializer()))
- && ((partitionStateSerializerConfigSnapshot == null && ((Snapshot) obj).getPartitionStateSerializerConfigSnapshot() == null)
- || partitionStateSerializerConfigSnapshot.equals(((Snapshot) obj).getPartitionStateSerializerConfigSnapshot()));
+ return name.equals(snapshot.getName())
+ && assignmentMode.equals(snapshot.getAssignmentMode())
+ && ((partitionStateSerializer == null && (snapshot.getPartitionStateSerializer() == null)
+ || (partitionStateSerializer != null && partitionStateSerializer.equals(snapshot.getPartitionStateSerializer())))
+ && ((partitionStateSerializerConfigSnapshot == null && (snapshot.getPartitionStateSerializerConfigSnapshot() == null)
+ || (partitionStateSerializerConfigSnapshot != null && partitionStateSerializerConfigSnapshot.equals(snapshot.getPartitionStateSerializerConfigSnapshot())));
--- End diff --
The parentheses of this condition check chain doesn't look right, leading to compilation errors.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4328: [FLINK-6493] Fix ineffective null check in RegisteredOper...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/4328
LGTM, merging ..
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4328: [FLINK-6493] Fix ineffective null check in Registe...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei commented on a diff in the pull request:
https://github.com/apache/flink/pull/4328#discussion_r131127614
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/state/RegisteredOperatorBackendStateMetaInfo.java ---
@@ -175,14 +176,19 @@ public boolean equals(Object obj) {
return false;
}
+ if (!(obj instanceof Snapshot)) {
+ return false;
+ }
+
+ Snapshot snapshot = (Snapshot)obj;
+
// need to check for nulls because serializer and config snapshots may be null on restore
- return (obj instanceof Snapshot)
- && name.equals(((Snapshot) obj).getName())
- && assignmentMode.equals(((Snapshot) obj).getAssignmentMode())
- && ((partitionStateSerializer == null && ((Snapshot) obj).getPartitionStateSerializer() == null)
- || partitionStateSerializer.equals(((Snapshot) obj).getPartitionStateSerializer()))
- && ((partitionStateSerializerConfigSnapshot == null && ((Snapshot) obj).getPartitionStateSerializerConfigSnapshot() == null)
- || partitionStateSerializerConfigSnapshot.equals(((Snapshot) obj).getPartitionStateSerializerConfigSnapshot()));
+ return Objects.equals(name, snapshot.getName())
+ && Objects.equals(assignmentMode, snapshot.getAssignmentMode())
+ && ((partitionStateSerializer == null && (snapshot.getPartitionStateSerializer() == null)
+ || (Objects.equals(partitionStateSerializer, snapshot.getPartitionStateSerializer())))
--- End diff --
Thanks you so much! Have been addressed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4328: [FLINK-6493] Fix ineffective null check in RegisteredOper...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei commented on the issue:
https://github.com/apache/flink/pull/4328
Sorry, Sir. I'll pay attention to the details of the next time.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4328: [FLINK-6493] Fix ineffective null check in RegisteredOper...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei commented on the issue:
https://github.com/apache/flink/pull/4328
Hi, @tzulitai I updated the code, I have to say, it is a very good method. Or, xxx.equals probably leading to NPE in our code. Especially in the production environment, people sometime forget make a check for Null.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4328: [FLINK-6493] Fix ineffective null check in Registe...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei commented on a diff in the pull request:
https://github.com/apache/flink/pull/4328#discussion_r131125429
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/state/RegisteredOperatorBackendStateMetaInfo.java ---
@@ -175,14 +176,19 @@ public boolean equals(Object obj) {
return false;
}
+ if (!(obj instanceof Snapshot)) {
+ return false;
+ }
+
+ Snapshot snapshot = (Snapshot)obj;
+
// need to check for nulls because serializer and config snapshots may be null on restore
- return (obj instanceof Snapshot)
- && name.equals(((Snapshot) obj).getName())
- && assignmentMode.equals(((Snapshot) obj).getAssignmentMode())
- && ((partitionStateSerializer == null && ((Snapshot) obj).getPartitionStateSerializer() == null)
- || partitionStateSerializer.equals(((Snapshot) obj).getPartitionStateSerializer()))
- && ((partitionStateSerializerConfigSnapshot == null && ((Snapshot) obj).getPartitionStateSerializerConfigSnapshot() == null)
- || partitionStateSerializerConfigSnapshot.equals(((Snapshot) obj).getPartitionStateSerializerConfigSnapshot()));
+ return Objects.equals(name, snapshot.getName())
+ && Objects.equals(assignmentMode, snapshot.getAssignmentMode())
--- End diff --
Yes. You are right. I did this because I want make them have the same way, Objects.equal. Now, I will return to the previously method. Thanks ~
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4328: [FLINK-6493] Fix ineffective null check in RegisteredOper...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei commented on the issue:
https://github.com/apache/flink/pull/4328
PR has been updated. Please helps to check again. Thanks ~
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4328: [FLINK-6493] Fix ineffective null check in RegisteredOper...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei commented on the issue:
https://github.com/apache/flink/pull/4328
Hi, @tzulitai I have removed the extra most-outer wrapping parenthesis by myself.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4328: [FLINK-6493] Fix ineffective null check in RegisteredOper...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei commented on the issue:
https://github.com/apache/flink/pull/4328
This time, build pass.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4328: [FLINK-6493] Fix ineffective null check in Registe...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on a diff in the pull request:
https://github.com/apache/flink/pull/4328#discussion_r131107143
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/state/RegisteredOperatorBackendStateMetaInfo.java ---
@@ -175,14 +176,19 @@ public boolean equals(Object obj) {
return false;
}
+ if (!(obj instanceof Snapshot)) {
+ return false;
+ }
+
+ Snapshot snapshot = (Snapshot)obj;
+
// need to check for nulls because serializer and config snapshots may be null on restore
- return (obj instanceof Snapshot)
- && name.equals(((Snapshot) obj).getName())
- && assignmentMode.equals(((Snapshot) obj).getAssignmentMode())
- && ((partitionStateSerializer == null && ((Snapshot) obj).getPartitionStateSerializer() == null)
- || partitionStateSerializer.equals(((Snapshot) obj).getPartitionStateSerializer()))
- && ((partitionStateSerializerConfigSnapshot == null && ((Snapshot) obj).getPartitionStateSerializerConfigSnapshot() == null)
- || partitionStateSerializerConfigSnapshot.equals(((Snapshot) obj).getPartitionStateSerializerConfigSnapshot()));
+ return Objects.equals(name, snapshot.getName())
+ && Objects.equals(assignmentMode, snapshot.getAssignmentMode())
--- End diff --
We don't need `null` checks for the assignment mode and name.
Those are already guaranteed to be non-null in the constructor.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4328: [FLINK-6493] Fix ineffective null check in Registe...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on a diff in the pull request:
https://github.com/apache/flink/pull/4328#discussion_r131068854
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/state/RegisteredOperatorBackendStateMetaInfo.java ---
@@ -175,14 +175,19 @@ public boolean equals(Object obj) {
return false;
}
+ if (!(obj instanceof Snapshot)) {
+ return false;
+ }
+
+ Snapshot snapshot = (Snapshot)obj;
+
// need to check for nulls because serializer and config snapshots may be null on restore
- return (obj instanceof Snapshot)
- && name.equals(((Snapshot) obj).getName())
- && assignmentMode.equals(((Snapshot) obj).getAssignmentMode())
- && ((partitionStateSerializer == null && ((Snapshot) obj).getPartitionStateSerializer() == null)
- || partitionStateSerializer.equals(((Snapshot) obj).getPartitionStateSerializer()))
- && ((partitionStateSerializerConfigSnapshot == null && ((Snapshot) obj).getPartitionStateSerializerConfigSnapshot() == null)
- || partitionStateSerializerConfigSnapshot.equals(((Snapshot) obj).getPartitionStateSerializerConfigSnapshot()));
+ return name.equals(snapshot.getName())
+ && assignmentMode.equals(snapshot.getAssignmentMode())
+ && ((partitionStateSerializer == null && (snapshot.getPartitionStateSerializer() == null)
--- End diff --
That makes sense. Haven't performed the merge yet, @zhangminglei would you like to address this comment?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4328: [FLINK-6493] Fix ineffective null check in RegisteredOper...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei commented on the issue:
https://github.com/apache/flink/pull/4328
Thanks @tzulitai. I updated the code, let us wait the build ending. :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4328: [FLINK-6493] Fix ineffective null check in RegisteredOper...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/4328
Thanks for that @zhangminglei. However, I've already finished testing a batch of commits with this fix included, and would like to merge that now. Sometimes I collect a batch of commits and run them on Travis before merging (that's why it isn't actually merged yet).
It's safe to leave some final minor cosmetic fix to the person merging the PR if they mentioned they'll fix it when merging. That'll also avoid duplicate work and wasted efforts.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4328: [FLINK-6493] Fix ineffective null check in Registe...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4328#discussion_r131068135
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/state/RegisteredOperatorBackendStateMetaInfo.java ---
@@ -175,14 +175,19 @@ public boolean equals(Object obj) {
return false;
}
+ if (!(obj instanceof Snapshot)) {
+ return false;
+ }
+
+ Snapshot snapshot = (Snapshot)obj;
+
// need to check for nulls because serializer and config snapshots may be null on restore
- return (obj instanceof Snapshot)
- && name.equals(((Snapshot) obj).getName())
- && assignmentMode.equals(((Snapshot) obj).getAssignmentMode())
- && ((partitionStateSerializer == null && ((Snapshot) obj).getPartitionStateSerializer() == null)
- || partitionStateSerializer.equals(((Snapshot) obj).getPartitionStateSerializer()))
- && ((partitionStateSerializerConfigSnapshot == null && ((Snapshot) obj).getPartitionStateSerializerConfigSnapshot() == null)
- || partitionStateSerializerConfigSnapshot.equals(((Snapshot) obj).getPartitionStateSerializerConfigSnapshot()));
+ return name.equals(snapshot.getName())
+ && assignmentMode.equals(snapshot.getAssignmentMode())
+ && ((partitionStateSerializer == null && (snapshot.getPartitionStateSerializer() == null)
--- End diff --
We could simplify this using `Objects.equals()`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4328: [FLINK-6493] Fix ineffective null check in Registe...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on a diff in the pull request:
https://github.com/apache/flink/pull/4328#discussion_r131167537
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/state/RegisteredOperatorBackendStateMetaInfo.java ---
@@ -175,14 +176,17 @@ public boolean equals(Object obj) {
return false;
}
+ if (!(obj instanceof Snapshot)) {
+ return false;
+ }
+
+ Snapshot snapshot = (Snapshot)obj;
+
// need to check for nulls because serializer and config snapshots may be null on restore
- return (obj instanceof Snapshot)
- && name.equals(((Snapshot) obj).getName())
- && assignmentMode.equals(((Snapshot) obj).getAssignmentMode())
- && ((partitionStateSerializer == null && ((Snapshot) obj).getPartitionStateSerializer() == null)
- || partitionStateSerializer.equals(((Snapshot) obj).getPartitionStateSerializer()))
- && ((partitionStateSerializerConfigSnapshot == null && ((Snapshot) obj).getPartitionStateSerializerConfigSnapshot() == null)
- || partitionStateSerializerConfigSnapshot.equals(((Snapshot) obj).getPartitionStateSerializerConfigSnapshot()));
+ return name.equals(snapshot.getName())
+ && assignmentMode.equals(snapshot.getAssignmentMode())
+ && (Objects.equals(partitionStateSerializer, snapshot.getPartitionStateSerializer()))
--- End diff --
These extra most-outer wrapping parenthesis are redundant.
I'll remove them when merging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4328: [FLINK-6493] Fix ineffective null check in RegisteredOper...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/4328
Thanks! Alright, this looks good now, merging (will address my final own comment while merging)!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4328: [FLINK-6493] Fix ineffective null check in Registe...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/4328
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4328: [FLINK-6493] Fix ineffective null check in Registe...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on a diff in the pull request:
https://github.com/apache/flink/pull/4328#discussion_r131097669
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/state/RegisteredOperatorBackendStateMetaInfo.java ---
@@ -175,14 +175,19 @@ public boolean equals(Object obj) {
return false;
}
+ if (!(obj instanceof Snapshot)) {
+ return false;
+ }
+
+ Snapshot snapshot = (Snapshot)obj;
+
// need to check for nulls because serializer and config snapshots may be null on restore
- return (obj instanceof Snapshot)
- && name.equals(((Snapshot) obj).getName())
- && assignmentMode.equals(((Snapshot) obj).getAssignmentMode())
- && ((partitionStateSerializer == null && ((Snapshot) obj).getPartitionStateSerializer() == null)
- || partitionStateSerializer.equals(((Snapshot) obj).getPartitionStateSerializer()))
- && ((partitionStateSerializerConfigSnapshot == null && ((Snapshot) obj).getPartitionStateSerializerConfigSnapshot() == null)
- || partitionStateSerializerConfigSnapshot.equals(((Snapshot) obj).getPartitionStateSerializerConfigSnapshot()));
+ return name.equals(snapshot.getName())
+ && assignmentMode.equals(snapshot.getAssignmentMode())
+ && ((partitionStateSerializer == null && (snapshot.getPartitionStateSerializer() == null)
--- End diff --
Cool. Please ping me again once updated!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4328: [FLINK-6493] Fix ineffective null check in RegisteredOper...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4328
Good fix, thanks!
I would suggest to improve the `equals(...)` method all together by pulling out the repeated casts to `Snapshot`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4328: [FLINK-6493] Fix ineffective null check in Registe...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on a diff in the pull request:
https://github.com/apache/flink/pull/4328#discussion_r131107573
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/state/RegisteredOperatorBackendStateMetaInfo.java ---
@@ -175,14 +176,19 @@ public boolean equals(Object obj) {
return false;
}
+ if (!(obj instanceof Snapshot)) {
+ return false;
+ }
+
+ Snapshot snapshot = (Snapshot)obj;
+
// need to check for nulls because serializer and config snapshots may be null on restore
- return (obj instanceof Snapshot)
- && name.equals(((Snapshot) obj).getName())
- && assignmentMode.equals(((Snapshot) obj).getAssignmentMode())
- && ((partitionStateSerializer == null && ((Snapshot) obj).getPartitionStateSerializer() == null)
- || partitionStateSerializer.equals(((Snapshot) obj).getPartitionStateSerializer()))
- && ((partitionStateSerializerConfigSnapshot == null && ((Snapshot) obj).getPartitionStateSerializerConfigSnapshot() == null)
- || partitionStateSerializerConfigSnapshot.equals(((Snapshot) obj).getPartitionStateSerializerConfigSnapshot()));
+ return Objects.equals(name, snapshot.getName())
+ && Objects.equals(assignmentMode, snapshot.getAssignmentMode())
+ && ((partitionStateSerializer == null && (snapshot.getPartitionStateSerializer() == null)
+ || (Objects.equals(partitionStateSerializer, snapshot.getPartitionStateSerializer())))
--- End diff --
```
&& ((partitionStateSerializer == null && (snapshot.getPartitionStateSerializer() == null)
|| (Objects.equals(partitionStateSerializer, snapshot.getPartitionStateSerializer())))
```
These two lines can just be `&& Objects.equals(partitionStateSerializer, snapshot.getPartitionStateSerializer())`, because the `Objects.equals` method returns `true` if both arguments are `null`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4328: [FLINK-6493] Fix ineffective null check in Registe...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei commented on a diff in the pull request:
https://github.com/apache/flink/pull/4328#discussion_r131071909
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/state/RegisteredOperatorBackendStateMetaInfo.java ---
@@ -175,14 +175,19 @@ public boolean equals(Object obj) {
return false;
}
+ if (!(obj instanceof Snapshot)) {
+ return false;
+ }
+
+ Snapshot snapshot = (Snapshot)obj;
+
// need to check for nulls because serializer and config snapshots may be null on restore
- return (obj instanceof Snapshot)
- && name.equals(((Snapshot) obj).getName())
- && assignmentMode.equals(((Snapshot) obj).getAssignmentMode())
- && ((partitionStateSerializer == null && ((Snapshot) obj).getPartitionStateSerializer() == null)
- || partitionStateSerializer.equals(((Snapshot) obj).getPartitionStateSerializer()))
- && ((partitionStateSerializerConfigSnapshot == null && ((Snapshot) obj).getPartitionStateSerializerConfigSnapshot() == null)
- || partitionStateSerializerConfigSnapshot.equals(((Snapshot) obj).getPartitionStateSerializerConfigSnapshot()));
+ return name.equals(snapshot.getName())
+ && assignmentMode.equals(snapshot.getAssignmentMode())
+ && ((partitionStateSerializer == null && (snapshot.getPartitionStateSerializer() == null)
--- End diff --
Yes. @tzulitai Hahaha, I am so flattered. Any comments is definitely welcomed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---