ZOOKEEPER-3223: Configure spotbugs - part 2

- move to spotbugs 3.1.9
- disable spotbugs on contrib package
- fix spotbugs warnings on recipes
- add commons-lang 2.6 dependency in order to fix build

Author: Enrico Olivelli <eolivelli@apache.org>

Reviewers: andor@apache.org

Closes #779 from eolivelli/fix/ZOOKEEPER-3223-master-part2

build.xml

@@ -28,7 +28,7 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant">
     <!-- ====================================================== -->
     <property name="slf4j.version" value="1.7.25"/>
     <property name="commons-cli.version" value="1.2"/>
-    <property name="spotbugsannotations.version" value="3.1.8"/>
+    <property name="spotbugsannotations.version" value="3.1.9"/>
 
     <property name="wagon-http.version" value="2.4"/>
     <property name="maven-ant-tasks.version" value="2.1.3"/>

pom.xml

@@ -278,7 +278,8 @@
     <kerby.version>1.1.0</kerby.version>
     <bouncycastle.version>1.56</bouncycastle.version>
     <commons-collections.version>3.2.2</commons-collections.version>
-    <spotbugsannotations.version>3.1.8</spotbugsannotations.version>
+    <commons-lang.version>2.6</commons-lang.version>
+    <spotbugsannotations.version>3.1.9</spotbugsannotations.version>
   </properties>
 
   <dependencyManagement>
@@ -293,6 +294,11 @@
         <artifactId>commons-collections</artifactId>
         <version>${commons-collections.version}</version>
       </dependency>
+      <dependency>
+        <groupId>commons-lang</groupId>
+        <artifactId>commons-lang</artifactId>
+        <version>${commons-lang.version}</version>
+      </dependency>
       <dependency>
         <groupId>org.apache.yetus</groupId>
         <artifactId>audience-annotations</artifactId>
@@ -472,7 +478,7 @@
         <plugin>
           <groupId>com.github.spotbugs</groupId>
           <artifactId>spotbugs-maven-plugin</artifactId>
-          <version>3.1.8</version>
+          <version>3.1.9</version>
           <configuration>
             <excludeFilterFile>excludeFindBugsFilter.xml</excludeFilterFile>
           </configuration>

zookeeper-contrib/pom.xml

@@ -34,11 +34,25 @@
   <description>
     Contrib projects to Apache ZooKeeper
   </description>
-
+  <build>
+    <pluginManagement>
+       <plugins>
+          <plugin>
+            <groupId>com.github.spotbugs</groupId>
+            <artifactId>spotbugs-maven-plugin</artifactId>
+            <version>3.1.9</version>
+            <configuration>
+                <!-- No spotbugs for contri modules -->
+                <skip>true</skip>
+            </configuration>
+          </plugin>
+        </plugins>
+      </pluginManagement>
+  </build>
   <modules>
     <module>zookeeper-contrib-loggraph</module>
     <module>zookeeper-contrib-rest</module>
     <module>zookeeper-contrib-zooinspector</module>
   </modules>
 
-</project>
+</project>

zookeeper-recipes/pom.xml

@@ -62,4 +62,4 @@
     <module>zookeeper-recipes-queue</module>
   </modules>
 
-</project>
+</project>

zookeeper-recipes/zookeeper-recipes-election/src/main/java/org/apache/zookeeper/recipes/leader/LeaderElectionSupport.java

@@ -180,27 +180,36 @@ public class LeaderElectionSupport implements Watcher {
     state = State.OFFER;
     dispatchEvent(EventType.OFFER_START);
 
-    leaderOffer = new LeaderOffer();
-
-    leaderOffer.setHostName(hostName);
-    leaderOffer.setNodePath(zooKeeper.create(rootNodeName + "/" + "n_",
-        hostName.getBytes(), ZooDefs.Ids.OPEN_ACL_UNSAFE,
+    LeaderOffer newLeaderOffer = new LeaderOffer();
+    byte[] hostnameBytes;
+    synchronized (this) {
+        newLeaderOffer.setHostName(hostName);
+        hostnameBytes = hostName.getBytes();
+        newLeaderOffer.setNodePath(zooKeeper.create(rootNodeName + "/" + "n_",
+        hostnameBytes, ZooDefs.Ids.OPEN_ACL_UNSAFE,
         CreateMode.EPHEMERAL_SEQUENTIAL));
-
+        leaderOffer = newLeaderOffer;
+    }
     logger.debug("Created leader offer {}", leaderOffer);
 
     dispatchEvent(EventType.OFFER_COMPLETE);
   }
 
+  private synchronized LeaderOffer getLeaderOffer() {
+      return leaderOffer;
+  }
+
   private void determineElectionStatus() throws KeeperException,
       InterruptedException {
 
     state = State.DETERMINE;
     dispatchEvent(EventType.DETERMINE_START);
 
-    String[] components = leaderOffer.getNodePath().split("/");
+    LeaderOffer currentLeaderOffer = getLeaderOffer();
+
+    String[] components = currentLeaderOffer.getNodePath().split("/");
 
-    leaderOffer.setId(Integer.valueOf(components[components.length - 1]
+    currentLeaderOffer.setId(Integer.valueOf(components[components.length - 1]
         .substring("n_".length())));
 
     List<LeaderOffer> leaderOffers = toLeaderOffers(zooKeeper.getChildren(
@@ -215,7 +224,7 @@ public class LeaderElectionSupport implements Watcher {
     for (int i = 0; i < leaderOffers.size(); i++) {
       LeaderOffer leaderOffer = leaderOffers.get(i);
 
-      if (leaderOffer.getId().equals(this.leaderOffer.getId())) {
+      if (leaderOffer.getId().equals(currentLeaderOffer.getId())) {
         logger.debug("There are {} leader offers. I am {} in line.",
             leaderOffers.size(), i);
 
@@ -237,7 +246,7 @@ public class LeaderElectionSupport implements Watcher {
       throws KeeperException, InterruptedException {
 
     logger.info("{} not elected leader. Watching node:{}",
-        leaderOffer.getNodePath(), neighborLeaderOffer.getNodePath());
+        getLeaderOffer().getNodePath(), neighborLeaderOffer.getNodePath());
 
     /*
      * Make sure to pass an explicit Watcher because we could be sharing this
@@ -270,7 +279,7 @@ public class LeaderElectionSupport implements Watcher {
     state = State.ELECTED;
     dispatchEvent(EventType.ELECTED_START);
 
-    logger.info("Becoming leader with node:{}", leaderOffer.getNodePath());
+    logger.info("Becoming leader with node:{}", getLeaderOffer().getNodePath());
 
     dispatchEvent(EventType.ELECTED_COMPLETE);
   }
@@ -336,7 +345,7 @@ public class LeaderElectionSupport implements Watcher {
   @Override
   public void process(WatchedEvent event) {
     if (event.getType().equals(Watcher.Event.EventType.NodeDeleted)) {
-      if (!event.getPath().equals(leaderOffer.getNodePath())
+      if (!event.getPath().equals(getLeaderOffer().getNodePath())
           && state != State.STOP) {
         logger.debug(
             "Node {} deleted. Need to run through the election process.",
@@ -384,8 +393,8 @@ public class LeaderElectionSupport implements Watcher {
 
   @Override
   public String toString() {
-    return "{ state:" + state + " leaderOffer:" + leaderOffer + " zooKeeper:"
-        + zooKeeper + " hostName:" + hostName + " listeners:" + listeners
+    return "{ state:" + state + " leaderOffer:" + getLeaderOffer() + " zooKeeper:"
+        + zooKeeper + " hostName:" + getHostName() + " listeners:" + listeners
         + " }";
   }
 
@@ -437,11 +446,11 @@ public class LeaderElectionSupport implements Watcher {
    * The hostname of this process. Mostly used as a convenience for logging and
    * to respond to {@link #getLeaderHostName()} requests.
    */
-  public String getHostName() {
+  public synchronized String getHostName() {
     return hostName;
   }
 
-  public void setHostName(String hostName) {
+  public synchronized void setHostName(String hostName) {
     this.hostName = hostName;
   }
 

zookeeper-recipes/zookeeper-recipes-election/src/main/java/org/apache/zookeeper/recipes/leader/LeaderOffer.java

@@ -16,6 +16,7 @@
  */
 package org.apache.zookeeper.recipes.leader;
 
+import java.io.Serializable;
 import java.util.Comparator;
 
 /**
@@ -72,7 +73,8 @@ public class LeaderOffer {
    * Compare two instances of {@link LeaderOffer} using only the {code}id{code}
    * member.
    */
-  public static class IdComparator implements Comparator<LeaderOffer> {
+  public static class IdComparator
+          implements Comparator<LeaderOffer>, Serializable {
 
     @Override
     public int compare(LeaderOffer o1, LeaderOffer o2) {

zookeeper-recipes/zookeeper-recipes-lock/pom.xml

@@ -40,6 +40,13 @@
       <artifactId>zookeeper-server</artifactId>
       <version>3.6.0-SNAPSHOT</version>
     </dependency>
+    <dependency>
+      <groupId>com.github.spotbugs</groupId>
+      <artifactId>spotbugs-annotations</artifactId>
+      <version>3.1.9</version>
+      <scope>provided</scope>
+      <optional>true</optional>
+    </dependency>
     <dependency>
       <groupId>org.apache.zookeeper</groupId>
       <artifactId>zookeeper-server</artifactId>
@@ -71,4 +78,4 @@
     </plugins>
   </build>
 
-</project>
+</project>

zookeeper-recipes/zookeeper-recipes-lock/src/main/java/org/apache/zookeeper/recipes/lock/WriteLock.java

@@ -17,6 +17,7 @@
  */
 package org.apache.zookeeper.recipes.lock;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.apache.zookeeper.KeeperException;
@@ -85,7 +86,7 @@ public class WriteLock extends ProtocolSupport {
      * return the current locklistener
      * @return the locklistener
      */
-    public LockListener getLockListener() {
+    public synchronized LockListener getLockListener() {
         return this.callback;
     }
     
@@ -93,7 +94,7 @@ public class WriteLock extends ProtocolSupport {
      * register a different call back listener
      * @param callback the call back instance
      */
-    public void setLockListener(LockListener callback) {
+    public synchronized void setLockListener(LockListener callback) {
         this.callback = callback;
     }
 
@@ -133,8 +134,9 @@ public class WriteLock extends ProtocolSupport {
                     initCause(e);
             }
             finally {
-                if (callback != null) {
-                    callback.lockReleased();
+                LockListener lockListener = getLockListener();
+                if (lockListener != null) {
+                    lockListener.lockReleased();
                 }
                 id = null;
             }
@@ -201,6 +203,8 @@ public class WriteLock extends ProtocolSupport {
          * obtaining the lock
          * @return if the command was successful or not
          */
+        @SuppressFBWarnings(value = "NP_NULL_PARAM_DEREF_NONVIRTUAL",
+                justification = "findPrefixInChildren will assign a value to this.id")
         public boolean execute() throws KeeperException, InterruptedException {
             do {
                 if (id == null) {
@@ -211,41 +215,40 @@ public class WriteLock extends ProtocolSupport {
                     findPrefixInChildren(prefix, zookeeper, dir);
                     idName = new ZNodeName(id);
                 }
-                if (id != null) {
-                    List<String> names = zookeeper.getChildren(dir, false);
-                    if (names.isEmpty()) {
-                        LOG.warn("No children in: " + dir + " when we've just " +
-                        "created one! Lets recreate it...");
-                        // lets force the recreation of the id
-                        id = null;
-                    } else {
-                        // lets sort them explicitly (though they do seem to come back in order ususally :)
-                        SortedSet<ZNodeName> sortedNames = new TreeSet<ZNodeName>();
-                        for (String name : names) {
-                            sortedNames.add(new ZNodeName(dir + "/" + name));
+                List<String> names = zookeeper.getChildren(dir, false);
+                if (names.isEmpty()) {
+                    LOG.warn("No children in: " + dir + " when we've just " +
+                    "created one! Lets recreate it...");
+                    // lets force the recreation of the id
+                    id = null;
+                } else {
+                    // lets sort them explicitly (though they do seem to come back in order ususally :)
+                    SortedSet<ZNodeName> sortedNames = new TreeSet<ZNodeName>();
+                    for (String name : names) {
+                        sortedNames.add(new ZNodeName(dir + "/" + name));
+                    }
+                    ownerId = sortedNames.first().getName();
+                    SortedSet<ZNodeName> lessThanMe = sortedNames.headSet(idName);
+                    if (!lessThanMe.isEmpty()) {
+                        ZNodeName lastChildName = lessThanMe.last();
+                        lastChildId = lastChildName.getName();
+                        if (LOG.isDebugEnabled()) {
+                            LOG.debug("watching less than me node: " + lastChildId);
                         }
-                        ownerId = sortedNames.first().getName();
-                        SortedSet<ZNodeName> lessThanMe = sortedNames.headSet(idName);
-                        if (!lessThanMe.isEmpty()) {
-                            ZNodeName lastChildName = lessThanMe.last();
-                            lastChildId = lastChildName.getName();
-                            if (LOG.isDebugEnabled()) {
-                                LOG.debug("watching less than me node: " + lastChildId);
-                            }
-                            Stat stat = zookeeper.exists(lastChildId, new LockWatcher());
-                            if (stat != null) {
-                                return Boolean.FALSE;
-                            } else {
-                                LOG.warn("Could not find the" +
-                                		" stats for less than me: " + lastChildName.getName());
-                            }
+                        Stat stat = zookeeper.exists(lastChildId, new LockWatcher());
+                        if (stat != null) {
+                            return Boolean.FALSE;
                         } else {
-                            if (isOwner()) {
-                                if (callback != null) {
-                                    callback.lockAcquired();
-                                }
-                                return Boolean.TRUE;
+                            LOG.warn("Could not find the" +
+                                            " stats for less than me: " + lastChildName.getName());
+                        }
+                    } else {
+                        if (isOwner()) {
+                            LockListener lockListener = getLockListener();
+                            if (lockListener != null) {
+                                lockListener.lockAcquired();
                             }
+                            return Boolean.TRUE;
                         }
                     }
                 }

zookeeper-recipes/zookeeper-recipes-queue/src/main/java/org/apache/zookeeper/recipes/queue/DistributedQueue.java

@@ -87,7 +87,7 @@ public class DistributedQueue {
                     continue;
                 }
                 String suffix = childName.substring(prefix.length());
-                Long childId = new Long(suffix);
+                Long childId = Long.parseLong(suffix);
                 orderedChildren.put(childId,childName);
             }catch(NumberFormatException e){
                 LOG.warn("Found child node with improper format : " + childName + " " + e,e);
@@ -209,7 +209,7 @@ public class DistributedQueue {
         }
     }
 
-    private class LatchChildWatcher implements Watcher {
+    private static class LatchChildWatcher implements Watcher {
 
         CountDownLatch latch;
 

zookeeper-server/pom.xml

@@ -50,6 +50,10 @@
       <artifactId>commons-collections</artifactId>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>commons-lang</groupId>
+      <artifactId>commons-lang</artifactId>
+    </dependency>
     <dependency>
       <groupId>org.apache.zookeeper</groupId>
       <artifactId>zookeeper-jute</artifactId>
@@ -267,4 +271,4 @@
     </plugins>
   </build>
 
-</project>
+</project>

zookeeper-server/src/main/java/org/apache/zookeeper/server/FinalRequestProcessor.java

@@ -505,7 +505,7 @@ public class FinalRequestProcessor implements RequestProcessor {
                 // so these values are passed along with the response.
                 GetDataResponse getDataResponse = (GetDataResponse)rsp;
                 Stat stat = null;
-                if (getDataResponse != null && getDataResponse.getStat() != null) {
+                if (getDataResponse.getStat() != null) {
                     stat = getDataResponse.getStat();
                 }
                 cnxn.sendResponse(hdr, rsp, "response", path, stat);