Shireesh Anjal has posted comments on this change.

Change subject: engine: Refresh gluster data periodically
......................................................................


Patch Set 8: (7 inline comments)

Responses to most comments in-line. New patch-set to follow with one comment 
incorporated (removing log from InitBackendServicesOnStartupBean as it is 
already happening from the GlusterManager#init())

....................................................
File backend/manager/modules/bll/pom.xml
Line 53:     </dependency>
Line 54: 
Line 55:     <dependency>
Line 56:       <groupId>${engine.groupId}</groupId>
Line 57:       <artifactId>dal</artifactId>
It's a 'test' dependency, as I use data from fixtures.xml for testing the 
functionality. Let me know if there is a better way.
Line 58:       <version>${engine.version}</version>
Line 59:       <type>test-jar</type>
Line 60:       <scope>test</scope>
Line 61:     </dependency>


....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterManager.java
Line 199:      * @return
Line 200:      */
Line 201:     private boolean isRemovableStatus(VDSStatus status) {
Line 202:         switch (status) {
Line 203:         case Up:
As mentioned before, this is how engine-code-formatter formats the switch-cases.
Line 204:         case Down:
Line 205:         case Maintenance:
Line 206:             return true;
Line 207:         default:


Line 228:     }
Line 229: 
Line 230:     private List<String> getVdsIps(VDS vds) {
Line 231:         List<String> vdsIps = new ArrayList<String>();
Line 232:         for (VdsNetworkInterface iface : 
getInterfaceDao().getAllInterfacesForVds(vds.getId())) {
addAll() can be used if vdsIps was List<VdsNetworkInterface>. However it is 
List<String> and hence I can't use addAll().
Line 233:             vdsIps.add(iface.getAddress());
Line 234:         }
Line 235:         return vdsIps;
Line 236:     }


Line 386:         for (GlusterVolumeEntity volume : 
getVolumeDao().getByClusterId(clusterId)) {
Line 387:             if (!volumesMap.containsKey(volume.getName())) {
Line 388:                 log.debugFormat("Volume {0} has been removed directly 
using the gluster CLI. Removing it from engine as well.",
Line 389:                         volume.getName());
Line 390:                 getVolumeDao().remove(volume.getId());
MassOperationsDAO supports only updateAll() operations currently. I could add a 
removeAll() method, provided you recommend it. Note that if we do this, we will 
first have to prepare a list of items to be removed using the same for loop, 
and then invoke the removeAll() operation on the Dao.

Please confirm.
Line 391:                 logVolumeMessage(volume, 
AuditLogType.GLUSTER_VOLUME_DELETED_FROM_CLI);
Line 392:             }
Line 393:         }
Line 394:     }


....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/InitBackendServicesOnStartupBean.java
Line 54:         });
Line 55:         StoragePoolStatusHandler.Init();
Line 56: 
Line 57:         GlusterManager.getInstance().init();
Line 58:         log.infoFormat("GlusterManager: {0}", new Date());
Done
Line 59:     }
Line 60: 


....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/SetNonOperationalVdsCommand.java
Line 75:         for (Entry<String, String> e : 
getParameters().getCustomLogValues().entrySet()) {
Line 76:             AddCustomValue(e.getKey(), e.getValue());
Line 77:         }
Line 78:         switch (getParameters().getNonOperationalReason()) {
Line 79:         case NETWORK_UNREACHABLE:
This is how the engine-code-formatter formats switch-cases (case statements at 
the same level as switch statement). This is because of following line in the 
engine-code-format.xml:

<setting 
id="org.eclipse.jdt.core.formatter.indent_switchstatements_compare_to_switch" 
value="false"/>

Changing the value to "true" will indent the case statements inside the switch 
block. So it looks like it was intentionally kept like this. If you think it 
was a mistake, I could send a patch to correct this in the formatter.
Line 80:             return (getSucceeded()) ? 
AuditLogType.VDS_SET_NONOPERATIONAL_NETWORK
Line 81:                     : AuditLogType.VDS_SET_NONOPERATIONAL_FAILED;
Line 82:         case STORAGE_DOMAIN_UNREACHABLE:
Line 83:             return (getSucceeded()) ? 
AuditLogType.VDS_SET_NONOPERATIONAL_DOMAIN


....................................................
File 
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/AuditLogType.java
Line 199:     GLUSTER_VOLUME_REPLACE_BRICK_START_FAILED(4018),
Line 200:     GLUSTER_VOLUME_ADD_BRICK(4019),
Line 201:     GLUSTER_VOLUME_ADD_BRICK_FAILED(4020),
Line 202:     GLUSTER_HOST_REMOVE_FAILED(4021),
Line 203:     GLUSTER_VOLUME_CREATED_FROM_CLI(4022),
This is the gluster CLI. The Gluster CLI can be used from any of the nodes of 
the cluster to perform gluster cluster operations. In fact all our gluster 
related features from the webadmin ultimately end up invoking the same gluster 
CLI to manage the gluster cluster. However if user performs any operations 
directly from the CLI (without using webadmin), such changes will be detected 
by the GlusterManager, synced with the engine DB and audited using these audit 
log types.
Line 204:     GLUSTER_VOLUME_DELETED_FROM_CLI(4023),
Line 205:     GLUSTER_VOLUME_OPTION_SET_FROM_CLI(4024),
Line 206:     GLUSTER_VOLUME_OPTION_RESET_FROM_CLI(4025),
Line 207:     GLUSTER_VOLUME_PROPERTIES_CHANGED_FROM_CLI(4026),


--
To view, visit http://gerrit.ovirt.org/7288
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b61eb6e93105e46e2706eac1d94bc10717224c2
Gerrit-PatchSet: 8
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Shireesh Anjal <san...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Shireesh Anjal <san...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to