etan39
/
cloudstack
mirror of https://github.com/apache/cloudstack.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

FindBugs findings: fixing equals() methods in 2 classes; commenting out dead variable in 1 class; adding 5 tests to cover the changes in the equals() methods. 

Signed-off-by: Hugo Trippaers <htrippaers@schubergphilis.com>
This commit is contained in:
wrodrigues 2014-02-15 13:23:35 +01:00 committed by Hugo Trippaers
parent abf3c055c2
commit 3a7e4103fc
5 changed files with 155 additions and 52 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

40
engine/orchestration/src/com/cloud/agent/manager/AgentAttache.java
View File

@ -32,9 +32,8 @@ import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;
import org.apache.log4j.Logger;
import org.apache.cloudstack.managed.context.ManagedContextRunnable;
import org.apache.log4j.Logger;
import com.cloud.agent.Listener;
import com.cloud.agent.api.Answer;
@ -72,7 +71,7 @@ public abstract class AgentAttache {
protected static final Comparator<Request> s_reqComparator = new Comparator<Request>() {
@Override
public int compare(Request o1, Request o2) {
public int compare(final Request o1, final Request o2) {
long seq1 = o1.getSequence();
long seq2 = o2.getSequence();
if (seq1 < seq2) {
@ -87,7 +86,7 @@ public abstract class AgentAttache {
protected static final Comparator<Object> s_seqComparator = new Comparator<Object>() {
@Override
public int compare(Object o1, Object o2) {
public int compare(final Object o1, final Object o2) {
long seq1 = ((Request)o1).getSequence();
long seq2 = (Long)o2;
if (seq1 < seq2) {
@ -122,7 +121,7 @@ public abstract class AgentAttache {
Arrays.sort(s_commandsNotAllowedInConnectingMode);
}
protected AgentAttache(AgentManagerImpl agentMgr, final long id, final String name, boolean maintenance) {
protected AgentAttache(final AgentManagerImpl agentMgr, final long id, final String name, final boolean maintenance) {
_id = id;
_name = name;
_waitForList = new ConcurrentHashMap<Long, Listener>();
@ -180,13 +179,13 @@ public abstract class AgentAttache {
}
}
protected synchronized void addRequest(Request req) {
protected synchronized void addRequest(final Request req) {
int index = findRequest(req);
assert (index < 0) : "How can we get index again? " + index + ":" + req.toString();
_requests.add(-index - 1, req);
}
protected void cancel(Request req) {
protected void cancel(final Request req) {
long seq = req.getSequence();
cancel(seq);
}
@ -205,11 +204,11 @@ public abstract class AgentAttache {
}
}
protected synchronized int findRequest(Request req) {
protected synchronized int findRequest(final Request req) {
return Collections.binarySearch(_requests, req, s_reqComparator);
}
protected synchronized int findRequest(long seq) {
protected synchronized int findRequest(final long seq) {
return Collections.binarySearch(_requests, seq, s_seqComparator);
}
@ -331,17 +330,20 @@ public abstract class AgentAttache {
}
@Override
public boolean equals(Object obj) {
try {
AgentAttache that = (AgentAttache)obj;
return _id == that._id;
} catch (ClassCastException e) {
assert false : "Who's sending an " + obj.getClass().getSimpleName() + " to AgentAttache.equals()? ";
public boolean equals(final Object obj) {
// Return false straight away.
if (obj == null) {
return false;
}
// No need to handle a ClassCastException. If the classes are different, then equals can return false straight ahead.
if (this.getClass() != obj.getClass()) {
return false;
}
AgentAttache that = (AgentAttache)obj;
return _id == that._id;
}
public void send(Request req, final Listener listener) throws AgentUnavailableException {
public void send(final Request req, final Listener listener) throws AgentUnavailableException {
checkAvailability(req.getCommands());
long seq = req.getSequence();
@ -387,7 +389,7 @@ public abstract class AgentAttache {
}
}
public Answer[] send(Request req, int wait) throws AgentUnavailableException, OperationTimedoutException {
public Answer[] send(final Request req, final int wait) throws AgentUnavailableException, OperationTimedoutException {
SynchronousListener sl = new SynchronousListener(null);
long seq = req.getSequence();
@ -478,7 +480,7 @@ public abstract class AgentAttache {
_currentSequence = req.getSequence();
}
public void process(Answer[] answers) {
public void process(final Answer[] answers) {
//do nothing
}
@ -505,7 +507,7 @@ public abstract class AgentAttache {
protected class Alarm extends ManagedContextRunnable {
long _seq;
public Alarm(long seq) {
public Alarm(final long seq) {
_seq = seq;
}

18
engine/orchestration/src/com/cloud/agent/manager/ClusteredAgentAttache.java
View File

@ -42,17 +42,17 @@ public class ClusteredAgentAttache extends ConnectedAgentAttache implements Rout
protected final LinkedList<Request> _transferRequests;
protected boolean _transferMode = false;
static public void initialize(ClusteredAgentManagerImpl agentMgr) {
static public void initialize(final ClusteredAgentManagerImpl agentMgr) {
s_clusteredAgentMgr = agentMgr;
}
public ClusteredAgentAttache(AgentManagerImpl agentMgr, long id, String name) {
public ClusteredAgentAttache(final AgentManagerImpl agentMgr, final long id, final String name) {
super(agentMgr, id, name, null, false);
_forward = true;
_transferRequests = new LinkedList<Request>();
}
public ClusteredAgentAttache(AgentManagerImpl agentMgr, long id, String name, Link link, boolean maintenance) {
public ClusteredAgentAttache(final AgentManagerImpl agentMgr, final long id, final String name, final Link link, final boolean maintenance) {
super(agentMgr, id, name, link, maintenance);
_forward = link == null;
_transferRequests = new LinkedList<Request>();
@ -84,7 +84,7 @@ public class ClusteredAgentAttache extends ConnectedAgentAttache implements Rout
}
@Override
public void cancel(long seq) {
public void cancel(final long seq) {
if (forForward()) {
Listener listener = getListener(seq);
if (listener != null && listener instanceof SynchronousListener) {
@ -106,7 +106,7 @@ public class ClusteredAgentAttache extends ConnectedAgentAttache implements Rout
}
@Override
public void routeToAgent(byte[] data) throws AgentUnavailableException {
public void routeToAgent(final byte[] data) throws AgentUnavailableException {
if (s_logger.isDebugEnabled()) {
s_logger.debug(log(Request.getSequence(data), "Routing from " + Request.getManagementServerId(data)));
}
@ -136,7 +136,7 @@ public class ClusteredAgentAttache extends ConnectedAgentAttache implements Rout
}
@Override
public void send(Request req, Listener listener) throws AgentUnavailableException {
public void send(final Request req, final Listener listener) throws AgentUnavailableException {
if (_link != null) {
super.send(req, listener);
return;
@ -220,7 +220,7 @@ public class ClusteredAgentAttache extends ConnectedAgentAttache implements Rout
_transferMode = transfer;
}
public boolean getTransferMode() {
public synchronized boolean getTransferMode() {
return _transferMode;
}
@ -232,13 +232,13 @@ public class ClusteredAgentAttache extends ConnectedAgentAttache implements Rout
}
}
protected synchronized void addRequestToTransfer(Request req) {
protected synchronized void addRequestToTransfer(final Request req) {
int index = findTransferRequest(req);
assert (index < 0) : "How can we get index again? " + index + ":" + req.toString();
_transferRequests.add(-index - 1, req);
}
protected synchronized int findTransferRequest(Request req) {
protected synchronized int findTransferRequest(final Request req) {
return Collections.binarySearch(_transferRequests, req, s_reqComparator);
}

33
engine/orchestration/src/com/cloud/agent/manager/ConnectedAgentAttache.java
View File

@ -33,13 +33,13 @@ public class ConnectedAgentAttache extends AgentAttache {
protected Link _link;
public ConnectedAgentAttache(AgentManagerImpl agentMgr, final long id, final String name, final Link link, boolean maintenance) {
public ConnectedAgentAttache(final AgentManagerImpl agentMgr, final long id, final String name, final Link link, final boolean maintenance) {
super(agentMgr, id, name, maintenance);
_link = link;
}
@Override
public synchronized void send(Request req) throws AgentUnavailableException {
public synchronized void send(final Request req) throws AgentUnavailableException {
try {
_link.send(req.toBytes());
} catch (ClosedChannelException e) {
@ -67,14 +67,31 @@ public class ConnectedAgentAttache extends AgentAttache {
}
@Override
public boolean equals(Object obj) {
try {
ConnectedAgentAttache that = (ConnectedAgentAttache)obj;
return super.equals(obj) && _link == that._link && _link != null;
} catch (ClassCastException e) {
assert false : "Who's sending an " + obj.getClass().getSimpleName() + " to " + this.getClass().getSimpleName() + ".equals()? ";
public int hashCode() {
final int prime = 31;
int result = 1;
result = prime * result + ((_link == null) ? 0 : _link.hashCode());
return result;
}
@Override
public boolean equals(final Object obj) {
// Return false straight away.
if (obj == null) {
return false;
}
// No need to handle a ClassCastException. If the classes are different, then equals can return false straight ahead.
if (this.getClass() != obj.getClass()) {
return false;
}
// This should not be part of the equals() method, but I'm keeping it because it is expected behaviour based
// on the previous implementation. The link attribute of the other object should be checked here as well
// to verify if it's not null whilst the this is null.
if (_link == null) {
return false;
}
ConnectedAgentAttache that = (ConnectedAgentAttache)obj;
return super.equals(obj) && _link == that._link;
}
@Override

34
engine/orchestration/src/org/apache/cloudstack/engine/service/api/ProvisioningServiceImpl.java
View File

@ -25,9 +25,6 @@ import java.util.Map;
import javax.inject.Inject;
import javax.ws.rs.Path;
import org.springframework.stereotype.Component;
import org.springframework.stereotype.Service;
import org.apache.cloudstack.engine.datacenter.entity.api.ClusterEntity;
import org.apache.cloudstack.engine.datacenter.entity.api.ClusterEntityImpl;
import org.apache.cloudstack.engine.datacenter.entity.api.DataCenterResourceManager;
@ -38,6 +35,8 @@ import org.apache.cloudstack.engine.datacenter.entity.api.PodEntityImpl;
import org.apache.cloudstack.engine.datacenter.entity.api.StorageEntity;
import org.apache.cloudstack.engine.datacenter.entity.api.ZoneEntity;
import org.apache.cloudstack.engine.datacenter.entity.api.ZoneEntityImpl;
import org.springframework.stereotype.Component;
import org.springframework.stereotype.Service;
import com.cloud.host.Host;
import com.cloud.host.Status;
@ -52,13 +51,13 @@ public class ProvisioningServiceImpl implements ProvisioningService {
DataCenterResourceManager manager;
@Override
public StorageEntity registerStorage(String name, List<String> tags, Map<String, String> details) {
public StorageEntity registerStorage(final String name, final List<String> tags, final Map<String, String> details) {
// TODO Auto-generated method stub
return null;
}
@Override
public ZoneEntity registerZone(String zoneUuid, String name, String owner, List<String> tags, Map<String, String> details) {
public ZoneEntity registerZone(final String zoneUuid, final String name, final String owner, final List<String> tags, final Map<String, String> details) {
ZoneEntityImpl zoneEntity = new ZoneEntityImpl(zoneUuid, manager);
zoneEntity.setName(name);
zoneEntity.setOwner(owner);
@ -68,7 +67,7 @@ public class ProvisioningServiceImpl implements ProvisioningService {
}
@Override
public PodEntity registerPod(String podUuid, String name, String owner, String zoneUuid, List<String> tags, Map<String, String> details) {
public PodEntity registerPod(final String podUuid, final String name, final String owner, final String zoneUuid, final List<String> tags, final Map<String, String> details) {
PodEntityImpl podEntity = new PodEntityImpl(podUuid, manager);
podEntity.setOwner(owner);
podEntity.setName(name);
@ -77,7 +76,7 @@ public class ProvisioningServiceImpl implements ProvisioningService {
}
@Override
public ClusterEntity registerCluster(String clusterUuid, String name, String owner, List<String> tags, Map<String, String> details) {
public ClusterEntity registerCluster(final String clusterUuid, final String name, final String owner, final List<String> tags, final Map<String, String> details) {
ClusterEntityImpl clusterEntity = new ClusterEntityImpl(clusterUuid, manager);
clusterEntity.setOwner(owner);
clusterEntity.setName(name);
@ -86,7 +85,7 @@ public class ProvisioningServiceImpl implements ProvisioningService {
}
@Override
public HostEntity registerHost(String hostUuid, String name, String owner, List<String> tags, Map<String, String> details) {
public HostEntity registerHost(final String hostUuid, final String name, final String owner, final List<String> tags, final Map<String, String> details) {
HostEntityImpl hostEntity = new HostEntityImpl(hostUuid, manager);
hostEntity.setOwner(owner);
hostEntity.setName(name);
@ -97,38 +96,38 @@ public class ProvisioningServiceImpl implements ProvisioningService {
}
@Override
public void deregisterStorage(String uuid) {
public void deregisterStorage(final String uuid) {
// TODO Auto-generated method stub
}
@Override
public void deregisterZone(String uuid) {
public void deregisterZone(final String uuid) {
ZoneEntityImpl zoneEntity = new ZoneEntityImpl(uuid, manager);
zoneEntity.disable();
}
@Override
public void deregisterPod(String uuid) {
public void deregisterPod(final String uuid) {
PodEntityImpl podEntity = new PodEntityImpl(uuid, manager);
podEntity.disable();
}
@Override
public void deregisterCluster(String uuid) {
public void deregisterCluster(final String uuid) {
ClusterEntityImpl clusterEntity = new ClusterEntityImpl(uuid, manager);
clusterEntity.disable();
}
@Override
public void deregisterHost(String uuid) {
public void deregisterHost(final String uuid) {
HostEntityImpl hostEntity = new HostEntityImpl(uuid, manager);
hostEntity.disable();
}
@Override
public void changeState(String type, String entity, Status state) {
public void changeState(final String type, final String entity, final Status state) {
// TODO Auto-generated method stub
}
@ -141,7 +140,10 @@ public class ProvisioningServiceImpl implements ProvisioningService {
@Override
public List<PodEntity> listPods() {
List<PodEntity> pods = new ArrayList<PodEntity>();
/*
* Not in use now, just commented out.
*/
//List<PodEntity> pods = new ArrayList<PodEntity>();
//pods.add(new PodEntityImpl("pod-uuid-1", "pod1"));
//pods.add(new PodEntityImpl("pod-uuid-2", "pod2"));
return null;
@ -162,7 +164,7 @@ public class ProvisioningServiceImpl implements ProvisioningService {
}
@Override
public ZoneEntity getZone(String uuid) {
public ZoneEntity getZone(final String uuid) {
ZoneEntityImpl impl = new ZoneEntityImpl(uuid, manager);
return impl;
}

82
engine/orchestration/test/com/cloud/agent/manager/ConnectedAgentAttacheTest.java Normal file
View File

@ -0,0 +1,82 @@
// Licensed to the Apache Software Foundation (ASF) under one
// or more contributor license agreements. See the NOTICE file
// distributed with this work for additional information
// regarding copyright ownership. The ASF licenses this file
// to you under the Apache License, Version 2.0 (the
// "License"); you may not use this file except in compliance
// with the License. You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing,
// software distributed under the License is distributed on an
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.
package com.cloud.agent.manager;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.mockito.Mockito.mock;
import org.junit.Test;
import com.cloud.utils.nio.Link;
public class ConnectedAgentAttacheTest {
@Test
public void testEquals() throws Exception {
Link link = mock(Link.class);
ConnectedAgentAttache agentAttache1 = new ConnectedAgentAttache(null, 0, null, link, false);
ConnectedAgentAttache agentAttache2 = new ConnectedAgentAttache(null, 0, null, link, false);
assertTrue(agentAttache1.equals(agentAttache2));
}
@Test
public void testEqualsFalseNull() throws Exception {
Link link = mock(Link.class);
ConnectedAgentAttache agentAttache1 = new ConnectedAgentAttache(null, 0, null, link, false);
assertFalse(agentAttache1.equals(null));
}
@Test
public void testEqualsFalseDiffLink() throws Exception {
Link link1 = mock(Link.class);
Link link2 = mock(Link.class);
ConnectedAgentAttache agentAttache1 = new ConnectedAgentAttache(null, 0, null, link1, false);
ConnectedAgentAttache agentAttache2 = new ConnectedAgentAttache(null, 0, null, link2, false);
assertFalse(agentAttache1.equals(agentAttache2));
}
@Test
public void testEqualsFalseDiffId() throws Exception {
Link link1 = mock(Link.class);
ConnectedAgentAttache agentAttache1 = new ConnectedAgentAttache(null, 1, null, link1, false);
ConnectedAgentAttache agentAttache2 = new ConnectedAgentAttache(null, 2, null, link1, false);
assertFalse(agentAttache1.equals(agentAttache2));
}
@Test
public void testEqualsFalseDiffClass() throws Exception {
Link link1 = mock(Link.class);
ConnectedAgentAttache agentAttache1 = new ConnectedAgentAttache(null, 1, null, link1, false);
assertFalse(agentAttache1.equals("abc"));
}
}