[GitHub] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...

[afine]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...

[rakeshadr]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...

[afine]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...

[afine]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...

[afine]

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 files = new ArrayList(Arrays.asList(txnLog
-.getDataDir().listFiles(new MyFileFilter(PREFIX_LOG;
+List files = new ArrayList();
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...

[afine]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...

[afine]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...

[rakeshadr]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...

[rakeshadr]

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 @@
 
 
 
+
--- 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...

[rakeshadr]

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 files = new ArrayList(Arrays.asList(txnLog
-.getDataDir().listFiles(new MyFileFilter(PREFIX_LOG;
+List files = new ArrayList();
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...

[rakeshadr]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...

[rakeshadr]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...

[rakeshadr]

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 files = new ArrayList(Arrays.asList(txnLog
-.getDataDir().listFiles(new MyFileFilter(PREFIX_LOG;
+List files = new ArrayList();
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...

[rakeshadr]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...

[rakeshadr]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #236: ZOOKEEPER-2733: Cleanup findbug warnings in bra...

[afine]

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 
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---