You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by 1m2c3t4 <gi...@git.apache.org> on 2017/10/08 07:38:34 UTC
[GitHub] flink pull request #4784: FLINK-7736: fix some lgtm.com alerts
GitHub user 1m2c3t4 opened a pull request:
https://github.com/apache/flink/pull/4784
FLINK-7736: fix some lgtm.com alerts
## What is the purpose of the change
lgtm.com performs deep analysis on more than 50,000 open source projects including many of the apache projects, identifying bugs and other opportunities for improvement of the code. This PR addresses 14 of the more straightforward ones found (see https://issues.apache.org/jira/browse/FLINK-7736 for details)
## Brief change log
Fixed the following alerts:
1) dereferenced variable is always null, in TaskSlotTable
2-3) array index out of bounds, in KVStateRequestSerializer and Utils
4) inconsistent equals and hashCode, in ArchivedJson
5-6) close input, in JarListHandler and SocketTextStreamFunction
7) close output, in JarFileCreator
8) unused format argument, in YarnApplicationMasterRunner
9) useless type test, in GroupReduceNode
10-11) useless comparison, in TaskExecutor and FieldAccessor
12-14) Result of integer multiplication cast to long, in MemoryManager and twice in InPlaceMutableHashTable
Also added a new test
## Verifying this change
This change added tests and can be verified as follows:
- *Added test that verifies that hashCode and equals are consistent in ArchivedJson
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: no
- The serializers: no
- The runtime per-record code paths (performance sensitive): no
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: no
## Documentation
- Does this pull request introduce a new feature? no
- If yes, how is the feature documented? not applicable
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/1m2c3t4/flink flink-7736
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/4784.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 #4784
----
commit 00801f9881a2a82411b77aff9e76e9a506f2bc71
Author: Malcolm Taylor <ma...@semmle.com>
Date: 2017-10-08T07:06:17Z
flink-7736: fix some lgtm.com alerts
----
---
[GitHub] flink pull request #4784: FLINK-7736: fix some lgtm.com alerts
Posted by haohui <gi...@git.apache.org>.
Github user haohui commented on a diff in the pull request:
https://github.com/apache/flink/pull/4784#discussion_r143363777
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/taskexecutor/slot/TaskSlotTable.java ---
@@ -448,7 +448,7 @@ public boolean addTask(Task task) throws SlotNotFoundException, SlotNotActiveExc
throw new SlotNotActiveException(task.getJobID(), task.getAllocationId());
}
} else {
- throw new SlotNotFoundException(taskSlot.getAllocationId());
+ throw new SlotNotFoundException(task.getAllocationId());
--- End diff --
doesn't seem to be equivalent?
---
[GitHub] flink pull request #4784: FLINK-7736: fix some lgtm.com alerts
Posted by 1m2c3t4 <gi...@git.apache.org>.
Github user 1m2c3t4 commented on a diff in the pull request:
https://github.com/apache/flink/pull/4784#discussion_r143418963
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/webmonitor/history/ArchivedJson.java ---
@@ -55,6 +57,13 @@ public boolean equals(Object obj) {
}
}
+ public int hashCode() {
--- End diff --
ok. I'll change this and push another commit. Thanks for the review @haohui
---
[GitHub] flink pull request #4784: FLINK-7736: fix some lgtm.com alerts
Posted by haohui <gi...@git.apache.org>.
Github user haohui commented on a diff in the pull request:
https://github.com/apache/flink/pull/4784#discussion_r143363755
--- Diff: flink-java/src/main/java/org/apache/flink/api/java/Utils.java ---
@@ -53,7 +53,7 @@ public static String getCallLocationName() {
public static String getCallLocationName(int depth) {
StackTraceElement[] stackTrace = Thread.currentThread().getStackTrace();
- if (stackTrace.length < depth) {
+ if (stackTrace.length <= depth) {
--- End diff --
why?
---
[GitHub] flink pull request #4784: FLINK-7736: fix some lgtm.com alerts
Posted by haohui <gi...@git.apache.org>.
Github user haohui commented on a diff in the pull request:
https://github.com/apache/flink/pull/4784#discussion_r143363797
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/webmonitor/history/ArchivedJson.java ---
@@ -55,6 +57,13 @@ public boolean equals(Object obj) {
}
}
+ public int hashCode() {
--- End diff --
Just use Objects.hash here. And it needs the override annotation.
---
[GitHub] flink issue #4784: FLINK-7736: fix some lgtm.com alerts
Posted by 1m2c3t4 <gi...@git.apache.org>.
Github user 1m2c3t4 commented on the issue:
https://github.com/apache/flink/pull/4784
This has been reviewed and approved, but not committed. Can one of the committers take a look please ?
---
[GitHub] flink pull request #4784: FLINK-7736: fix some lgtm.com alerts
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/4784#discussion_r143426558
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/taskexecutor/slot/TaskSlotTable.java ---
@@ -448,7 +448,7 @@ public boolean addTask(Task task) throws SlotNotFoundException, SlotNotActiveExc
throw new SlotNotActiveException(task.getJobID(), task.getAllocationId());
}
} else {
- throw new SlotNotFoundException(taskSlot.getAllocationId());
+ throw new SlotNotFoundException(task.getAllocationId());
--- End diff --
Yes, this is a bug. Thanks for fixing it.
---
[GitHub] flink pull request #4784: FLINK-7736: fix some lgtm.com alerts
Posted by haohui <gi...@git.apache.org>.
Github user haohui commented on a diff in the pull request:
https://github.com/apache/flink/pull/4784#discussion_r143363766
--- Diff: flink-optimizer/src/main/java/org/apache/flink/optimizer/dag/GroupReduceNode.java ---
@@ -97,7 +97,7 @@ else if (Optimizer.HINT_LOCAL_STRATEGY_COMBINING_SORT.equals(localStrategy)) {
// check if we can work with a grouping (simple reducer), or if we need ordering because of a group order
Ordering groupOrder = null;
- if (getOperator() instanceof GroupReduceOperatorBase) {
+ if (getOperator() != null) {
--- End diff --
doesn't seems to be equivalent?
---
[GitHub] flink issue #4784: FLINK-7736: fix some lgtm.com alerts
Posted by 1m2c3t4 <gi...@git.apache.org>.
Github user 1m2c3t4 commented on the issue:
https://github.com/apache/flink/pull/4784
Rebased again and fixed another obvious bug
---
[GitHub] flink pull request #4784: FLINK-7736: fix some lgtm.com alerts
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/4784
---
[GitHub] flink issue #4784: FLINK-7736: fix some lgtm.com alerts
Posted by 1m2c3t4 <gi...@git.apache.org>.
Github user 1m2c3t4 commented on the issue:
https://github.com/apache/flink/pull/4784
All review comments addressed
---
[GitHub] flink pull request #4784: FLINK-7736: fix some lgtm.com alerts
Posted by 1m2c3t4 <gi...@git.apache.org>.
Github user 1m2c3t4 commented on a diff in the pull request:
https://github.com/apache/flink/pull/4784#discussion_r143418356
--- Diff: flink-optimizer/src/main/java/org/apache/flink/optimizer/dag/GroupReduceNode.java ---
@@ -97,7 +97,7 @@ else if (Optimizer.HINT_LOCAL_STRATEGY_COMBINING_SORT.equals(localStrategy)) {
// check if we can work with a grouping (simple reducer), or if we need ordering because of a group order
Ordering groupOrder = null;
- if (getOperator() instanceof GroupReduceOperatorBase) {
+ if (getOperator() != null) {
--- End diff --
getOperator() has a return type of GroupReduceOperatorBase, so the only problematic case would be if it returns null
---
[GitHub] flink pull request #4784: FLINK-7736: fix some lgtm.com alerts
Posted by 1m2c3t4 <gi...@git.apache.org>.
Github user 1m2c3t4 commented on a diff in the pull request:
https://github.com/apache/flink/pull/4784#discussion_r143417796
--- Diff: flink-java/src/main/java/org/apache/flink/api/java/Utils.java ---
@@ -53,7 +53,7 @@ public static String getCallLocationName() {
public static String getCallLocationName(int depth) {
StackTraceElement[] stackTrace = Thread.currentThread().getStackTrace();
- if (stackTrace.length < depth) {
+ if (stackTrace.length <= depth) {
--- End diff --
Because the subsequent statement elem = stackTrace[depth]
will throw ArrayIndexOutOfBoundsException if stackTrace.length <= depth
---
[GitHub] flink pull request #4784: FLINK-7736: fix some lgtm.com alerts
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4784#discussion_r143407857
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/taskexecutor/slot/TaskSlotTable.java ---
@@ -448,7 +448,7 @@ public boolean addTask(Task task) throws SlotNotFoundException, SlotNotActiveExc
throw new SlotNotActiveException(task.getJobID(), task.getAllocationId());
}
} else {
- throw new SlotNotFoundException(taskSlot.getAllocationId());
+ throw new SlotNotFoundException(task.getAllocationId());
--- End diff --
this is in the else branch of `taskSlot != null`, as in it would always fail with a `NullPointerException`. @tillrohrmann
---
[GitHub] flink issue #4784: FLINK-7736: fix some lgtm.com alerts
Posted by 1m2c3t4 <gi...@git.apache.org>.
Github user 1m2c3t4 commented on the issue:
https://github.com/apache/flink/pull/4784
Rebased.
@zentol @tillrohrmann thanks for reviewing this. Is it ready for merge now ?
---