Sahina Bose has posted comments on this change.
Change subject: gluster: Sync gluster service statuses
......................................................................
Patch Set 4: (11 inline comments)
Shireesh, you seem to have covered most cases! Some comments regarding
readability of code.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterServiceSyncJob.java
Line 109: for (Entry<ServiceType, GlusterServiceStatus> entry :
fetchedClusterServiceStatusMap.entrySet()) {
Line 110: ServiceType type = entry.getKey();
Line 111: GlusterServiceStatus status = entry.getValue();
Line 112:
Line 113: GlusterClusterService foundService =
existingClusterServiceMap.get(type);
foundService or existingClusterService?
Line 114: if (foundService == null) {
Line 115: existingClusterServiceMap.put(type,
mapServiceTypeToCluster(cluster, type, status));
Line 116: } else if (foundService.getStatus() != status) {
Line 117: updateClusterServiceStatus(foundService, status);
Line 111: GlusterServiceStatus status = entry.getValue();
Line 112:
Line 113: GlusterClusterService foundService =
existingClusterServiceMap.get(type);
Line 114: if (foundService == null) {
Line 115: existingClusterServiceMap.put(type,
mapServiceTypeToCluster(cluster, type, status));
Not obvious from the method name mapServiceTypeToCluster that there's a
database insert happening as well. You could consider refactoring.
Line 116: } else if (foundService.getStatus() != status) {
Line 117: updateClusterServiceStatus(foundService, status);
Line 118: }
Line 119: }
Line 120: }
Line 121:
Line 122: private Map<ServiceType, GlusterServiceStatus>
createFetchedClusterServiceStatusMap(List<Map<String, GlusterServiceStatus>>
serviceStatusMaps) {
Line 123: Map<ServiceType, GlusterServiceStatus>
fetchedClusterServiceStatusMap =
Line 124: new HashMap<ServiceType, GlusterServiceStatus>();
or fetchedServiceTypeStatusMap?
Line 125: for (Entry<String, GlusterServiceStatus> entry :
mergeServiceStatusMaps(serviceStatusMaps).entrySet()) {
Line 126: String serviceName = entry.getKey();
Line 127: GlusterServiceStatus status = entry.getValue();
Line 128: ServiceType type =
getServiceNameMap().get(serviceName).getServiceType();
Line 130: GlusterServiceStatus foundStatus =
fetchedClusterServiceStatusMap.get(type);
Line 131: if (foundStatus == null) {
Line 132: fetchedClusterServiceStatusMap.put(type, status);
Line 133: } else {
Line 134: if (foundStatus != status) {
Won't else if (foundStatus != status) do?
Line 135: fetchedClusterServiceStatusMap.put(type,
GlusterServiceStatus.MIXED);
Line 136: }
Line 137: }
Line 138: }
Line 139: return fetchedClusterServiceStatusMap;
Line 140: }
Line 141:
Line 142: private Map<String, GlusterServiceStatus>
mergeServiceStatusMaps(List<Map<String, GlusterServiceStatus>>
serviceStatusMaps) {
Line 143: Map<String, GlusterServiceStatus> serviceStatusMap = new
HashMap<String, GlusterServiceStatus>();
Suggestion - could rename this to mergedServiceStatusMap and the pairResult in
the loop below to serviceStatusMap to follow this code better.
Line 144: for (Map<String, GlusterServiceStatus> pairResult :
serviceStatusMaps) {
Line 145: for (Entry<String, GlusterServiceStatus> entry :
pairResult.entrySet()) {
Line 146: String serviceName = entry.getKey();
Line 147: GlusterServiceStatus status = entry.getValue();
Line 178: */
Line 179: @SuppressWarnings({ "unchecked", "serial" })
Line 180: private Map<String, GlusterServiceStatus>
refreshServerServices(VDS server) {
Line 181: Map<String, GlusterServiceStatus> serviceStatusMap = new
HashMap<String, GlusterServiceStatus>();
Line 182:
Shouldn't clusterId be passed here, the method acquireLock seems to take
clusterId as parameter?
On another note, is cluster level locking required as we are only retrieving
services for a cluster and updating status. There's no gluster CLI command
invoked here.
Line 183: acquireLock(server.getId());
Line 184: try {
Line 185: Map<Guid, GlusterServerService> existingServicesMap =
getExistingServices(server);
Line 186: List<GlusterServerService> servicesToUpdate = new
ArrayList<GlusterServerService>();
Line 193: "Updating statuses of all services on this
server to UNKNOWN.",
Line 194: server.getHostName(),
Line 195: returnValue.getVdsError().getMessage());
Line 196: logUtil.logServerMessage(server,
AuditLogType.GLUSTER_SERVICES_LIST_FAILED);
Line 197: return
updateStatusToUnknown(existingServicesMap.values());
Could existingServicesMap be null?
Isn't it possible that the services are not installed?
In this case, should status be UNKNOWN or NOTAVAILABLE ?
Line 198: }
Line 199:
Line 200: for (final GlusterServerService fetchedService :
(List<GlusterServerService>) returnValue.getReturnValue()) {
Line 201: serviceStatusMap.put(fetchedService.getServiceName(),
fetchedService.getStatus());
Line 239:
Line 240: private Map<String, GlusterServiceStatus>
updateStatusToUnknown(Collection<GlusterServerService> existingServices) {
Line 241: Map<String, GlusterServiceStatus> serviceStatusMap = new
HashMap<String, GlusterServiceStatus>();
Line 242:
Line 243: List<GlusterServerService> servicesToUpdate = new
ArrayList<GlusterServerService>();
Is this list required? You could directly use existingServices list
Line 244: for (GlusterServerService existingService : existingServices)
{
Line 245: existingService.setStatus(GlusterServiceStatus.UNKNOWN);
Line 246: servicesToUpdate.add(existingService);
Line 247: serviceStatusMap.put(existingService.getServiceName(),
existingService.getStatus());
Line 249:
Line 250: getGlusterServerServiceDao().updateAll(servicesToUpdate);
Line 251: return serviceStatusMap;
Line 252: }
Line 253:
Could rename to getExistingServicesMap to be more readable
Line 254: private Map<Guid, GlusterServerService> getExistingServices(VDS
server) {
Line 255: List<GlusterServerService> existingServices =
getGlusterServerServiceDao().getByServerId(server.getId());
Line 256: Map<Guid, GlusterServerService> existingServicesMap = new
HashMap<Guid, GlusterServerService>();
Line 257: if (existingServices != null) {
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/gluster/GlusterServiceSyncJobTest.java
Line 225: public void testRefreshGlusterServicesWithChanges() throws
Exception {
Line 226: mockWithChanges();
Line 227: syncJob.refreshGlusterServices();
Line 228: verifyWithChanges();
Line 229: }
You could add a test for when the VDS command fails to execute - check if
status is updated in this case.
Line 230:
Line 231: private void verifyCommonCalls() {
Line 232: // all clusters fetched from db
Line 233: verify(clusterDao, times(1)).getAll();
Line 280:
Line 281: GlusterClusterService service =
(GlusterClusterService) argument;
Line 282: switch (service.getServiceType()) {
Line 283: case GLUSTER:
Line 284: return service.getStatus() ==
GlusterServiceStatus.STOPPED;
Shouldn't service status be mixed in case of GLUSTER service type - as it it
RUNNING on server1 and STOPPED on server3?
Line 285: case GLUSTER_SWIFT:
Line 286: return service.getStatus() ==
GlusterServiceStatus.MIXED;
Line 287: default:
Line 288: return false;
--
To view, visit http://gerrit.ovirt.org/14644
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I312cacf698cb3420e30d96c42a92959b257c4abd
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Shireesh Anjal <[email protected]>
Gerrit-Reviewer: Eli Mesika <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Sahina Bose <[email protected]>
Gerrit-Reviewer: Shireesh Anjal <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
