You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by afine <gi...@git.apache.org> on 2017/04/21 21:23:40 UTC
[GitHub] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...
GitHub user afine opened a pull request:
https://github.com/apache/zookeeper/pull/236
ZOOKEEPER-2733: Cleanup findbug warnings in branch-3.4: Dodgy code Warnings
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/afine/zookeeper ZOOKEEPER-2733
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/236.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 #236
----
commit d62b86a6b3fbbaf503a4c0e8b08c7fc0fbc2df70
Author: Abraham Fine <af...@apache.org>
Date: 2017-04-21T21:22:29Z
ZOOKEEPER-2733: Cleanup findbug warnings in branch-3.4: Dodgy code Warnings
----
---
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] zookeeper issue #236: ZOOKEEPER-2733: Cleanup findbug warnings in branch-3.4...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on the issue:
https://github.com/apache/zookeeper/pull/236
Thanks @afine for the contribution.
+1 LGTM, I'll commit shortly.
---
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] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/236#discussion_r117283048
--- Diff: src/java/main/org/apache/zookeeper/server/PurgeTxnLog.java ---
@@ -133,11 +133,17 @@ public boolean accept(File f){
}
}
// add all non-excluded log files
- List<File> files = new ArrayList<File>(Arrays.asList(txnLog
- .getDataDir().listFiles(new MyFileFilter(PREFIX_LOG))));
+ List<File> files = new ArrayList<File>();
+ File[] fileArray;
+ if ((fileArray = txnLog.getDataDir().listFiles(new MyFileFilter(PREFIX_LOG))) != null) {
+ files.addAll(Arrays.asList(fileArray));
+ }
+
// add all non-excluded snapshot files to the deletion list
- files.addAll(Arrays.asList(txnLog.getSnapDir().listFiles(
- new MyFileFilter(PREFIX_SNAPSHOT))));
+ if ((fileArray = txnLog.getSnapDir().listFiles(new MyFileFilter(PREFIX_SNAPSHOT))) != null) {
--- End diff --
Same as above, Its good to keep the var assignment outside if check for better readability.
`(fileArray = txnLog.getSnapDir().listFiles(new MyFileFilter(PREFIX_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] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/236#discussion_r117345541
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/Follower.java ---
@@ -135,6 +135,8 @@ protected void processPacket(QuorumPacket qp) throws IOException{
case Leader.SYNC:
fzk.sync();
break;
+ default:
+ LOG.error("Invalid packet type received by Observer: " + qp.getType());
--- End diff --
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] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/236#discussion_r117279935
--- Diff: src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java ---
@@ -628,14 +628,7 @@ protected void pRequest(Request request) throws RequestProcessorException {
break;
//All the rest don't need to create a Txn - just verify session
- case OpCode.sync:
- case OpCode.exists:
- case OpCode.getData:
- case OpCode.getACL:
- case OpCode.getChildren:
- case OpCode.getChildren2:
- case OpCode.ping:
- case OpCode.setWatches:
+ default:
--- End diff --
This will execute if any `unknown type` and is not expected, isn't it?
We could keep the existing case checks and add default like,
```
zks.sessionTracker.checkSession(request.sessionId,
request.getOwner());
break;
default:
LOG.warn("unknown type " + request.type);
break;
```
---
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] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/236#discussion_r117673526
--- Diff: src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java ---
@@ -628,14 +628,7 @@ protected void pRequest(Request request) throws RequestProcessorException {
break;
//All the rest don't need to create a Txn - just verify session
- case OpCode.sync:
- case OpCode.exists:
- case OpCode.getData:
- case OpCode.getACL:
- case OpCode.getChildren:
- case OpCode.getChildren2:
- case OpCode.ping:
- case OpCode.setWatches:
+ default:
--- End diff --
Thanks @afine for the detailed explanation. Yes, I'd prefer to keep the code in sync with branch-3.5/trunk. Also, this way, protects the code irrespective of upper layer code changes.
---
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] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/236#discussion_r117285488
--- Diff: src/java/main/org/apache/zookeeper/server/upgrade/UpgradeMain.java ---
@@ -113,16 +113,18 @@ private void createAllDirs() throws IOException {
* @throws IOException
*/
void copyFiles(File srcDir, File dstDir, String filter) throws IOException {
- File[] list = srcDir.listFiles();
- for (File file: list) {
- String name = file.getName();
- if (name.startsWith(filter)) {
- // we need to copy this file
- File dest = new File(dstDir, name);
- LOG.info("Renaming " + file + " to " + dest);
- if (!file.renameTo(dest)) {
- throw new IOException("Unable to rename "
- + file + " to " + dest);
+ File[] list;
+ if ((list = srcDir.listFiles()) != null) {
--- End diff --
Please keep var assignment `(list = srcDir.listFiles())` along with the object reference.
`File[] list = srcDir.listFiles();`
---
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] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/236#discussion_r117345587
--- Diff: src/java/main/org/apache/zookeeper/server/upgrade/UpgradeMain.java ---
@@ -113,16 +113,18 @@ private void createAllDirs() throws IOException {
* @throws IOException
*/
void copyFiles(File srcDir, File dstDir, String filter) throws IOException {
- File[] list = srcDir.listFiles();
- for (File file: list) {
- String name = file.getName();
- if (name.startsWith(filter)) {
- // we need to copy this file
- File dest = new File(dstDir, name);
- LOG.info("Renaming " + file + " to " + dest);
- if (!file.renameTo(dest)) {
- throw new IOException("Unable to rename "
- + file + " to " + dest);
+ File[] list;
+ if ((list = srcDir.listFiles()) != null) {
--- End diff --
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] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/236#discussion_r117280664
--- Diff: src/java/main/org/apache/zookeeper/server/PurgeTxnLog.java ---
@@ -133,11 +133,17 @@ public boolean accept(File f){
}
}
// add all non-excluded log files
- List<File> files = new ArrayList<File>(Arrays.asList(txnLog
- .getDataDir().listFiles(new MyFileFilter(PREFIX_LOG))));
+ List<File> files = new ArrayList<File>();
+ File[] fileArray;
+ if ((fileArray = txnLog.getDataDir().listFiles(new MyFileFilter(PREFIX_LOG))) != null) {
--- End diff --
Can we keep the var assignment `File[] fileArray` outside along with the object reference instead of clubbing with if check.
File[] fileArray = txnLog.getDataDir().listFiles(new MyFileFilter(PREFIX_LOG);
---
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] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/236#discussion_r117341551
--- Diff: src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java ---
@@ -628,14 +628,7 @@ protected void pRequest(Request request) throws RequestProcessorException {
break;
//All the rest don't need to create a Txn - just verify session
- case OpCode.sync:
- case OpCode.exists:
- case OpCode.getData:
- case OpCode.getACL:
- case OpCode.getChildren:
- case OpCode.getChildren2:
- case OpCode.ping:
- case OpCode.setWatches:
+ default:
--- End diff --
I get your point. I would argue that we perform the check to make sure we don't get a bad OpCode here: https://github.com/apache/zookeeper/blob/branch-3.4/src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java#L742 and I think this is cleaner. I would be willing to change this if you feel strongly about it.
---
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] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/236#discussion_r117345561
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/Observer.java ---
@@ -125,6 +125,8 @@ protected void processPacket(QuorumPacket qp) throws IOException{
ObserverZooKeeperServer obs = (ObserverZooKeeperServer)zk;
obs.commitRequest(request);
break;
+ default:
+ LOG.error("Invalid packet type received by Observer: " + qp.getType());
--- End diff --
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] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/236#discussion_r117284521
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/Observer.java ---
@@ -125,6 +125,8 @@ protected void processPacket(QuorumPacket qp) throws IOException{
ObserverZooKeeperServer obs = (ObserverZooKeeperServer)zk;
obs.commitRequest(request);
break;
+ default:
+ LOG.error("Invalid packet type received by Observer: " + qp.getType());
--- End diff --
Can we change log message like below to improve readability.
`LOG.error("Invalid packet type: {} received by Observer", qp.getType());`
---
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] zookeeper issue #236: ZOOKEEPER-2733: Cleanup findbug warnings in branch-3.4...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on the issue:
https://github.com/apache/zookeeper/pull/236
@rakeshadr all your concerns should now be 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] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/236#discussion_r117284290
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/Follower.java ---
@@ -135,6 +135,8 @@ protected void processPacket(QuorumPacket qp) throws IOException{
case Leader.SYNC:
fzk.sync();
break;
+ default:
+ LOG.error("Invalid packet type received by Observer: " + qp.getType());
--- End diff --
Can we change log message like below to improve readability.
`LOG.error("Invalid packet type: {} received by Observer", qp.getType());`
---
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] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/236#discussion_r117287341
--- Diff: ivy.xml ---
@@ -49,6 +49,8 @@
<dependency org="log4j" name="log4j" rev="1.2.16" transitive="false" conf="default"/>
<dependency org="jline" name="jline" rev="0.9.94" transitive="false" conf="default"/>
+ <dependency org="com.google.code.findbugs" name="annotations" rev="3.0.1" conf="default"/>
--- End diff --
I'd prefer to exclude `AuthFastLeaderElection.java` as this is deprecated rather than introducing google library dependency. Does this sound good to you. 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] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/236#discussion_r117345471
--- Diff: src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java ---
@@ -504,6 +504,8 @@ protected void pRequest2Txn(int type, long zxid, Request request, Record record,
version = currentVersion + 1;
request.txn = new CheckVersionTxn(path, version);
break;
+ default:
+ LOG.error("Invalid OpCode received by PrepRequestProcessor: " + type);
--- End diff --
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] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...
Posted by afine <gi...@git.apache.org>.
Github user afine closed the pull request at:
https://github.com/apache/zookeeper/pull/236
---
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] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/236#discussion_r117345530
--- Diff: src/java/main/org/apache/zookeeper/server/PurgeTxnLog.java ---
@@ -133,11 +133,17 @@ public boolean accept(File f){
}
}
// add all non-excluded log files
- List<File> files = new ArrayList<File>(Arrays.asList(txnLog
- .getDataDir().listFiles(new MyFileFilter(PREFIX_LOG))));
+ List<File> files = new ArrayList<File>();
+ File[] fileArray;
+ if ((fileArray = txnLog.getDataDir().listFiles(new MyFileFilter(PREFIX_LOG))) != null) {
+ files.addAll(Arrays.asList(fileArray));
+ }
+
// add all non-excluded snapshot files to the deletion list
- files.addAll(Arrays.asList(txnLog.getSnapDir().listFiles(
- new MyFileFilter(PREFIX_SNAPSHOT))));
+ if ((fileArray = txnLog.getSnapDir().listFiles(new MyFileFilter(PREFIX_SNAPSHOT))) != null) {
--- End diff --
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] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/236#discussion_r117232748
--- Diff: src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java ---
@@ -504,6 +504,8 @@ protected void pRequest2Txn(int type, long zxid, Request request, Record record,
version = currentVersion + 1;
request.txn = new CheckVersionTxn(path, version);
break;
+ default:
+ LOG.error("Invalid OpCode received by PrepRequestProcessor: " + type);
--- End diff --
Just a suggestion to make this more readable.
`LOG.error("Invalid OpCode: {} received by PrepRequestProcessor", type);`
---
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] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/236#discussion_r117345515
--- Diff: src/java/main/org/apache/zookeeper/server/PurgeTxnLog.java ---
@@ -133,11 +133,17 @@ public boolean accept(File f){
}
}
// add all non-excluded log files
- List<File> files = new ArrayList<File>(Arrays.asList(txnLog
- .getDataDir().listFiles(new MyFileFilter(PREFIX_LOG))));
+ List<File> files = new ArrayList<File>();
+ File[] fileArray;
+ if ((fileArray = txnLog.getDataDir().listFiles(new MyFileFilter(PREFIX_LOG))) != null) {
--- End diff --
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] zookeeper issue #236: ZOOKEEPER-2733: Cleanup findbug warnings in branch-3.4...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on the issue:
https://github.com/apache/zookeeper/pull/236
Merged, please close pr @afine
---
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.
---