Kanagaraj M has posted comments on this change.
Change subject: engine:Sync gluster hooks
......................................................................
Patch Set 2: (7 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterHookManager.java
Line 35:
Line 36: @OnTimerMethodAnnotation("refreshHooks")
Line 37: public void refreshHooks() {
Line 38: log.debug("Refreshing hooks list");
Line 39: List<VDSGroup> clusters = getClusterDao().getAll();
only the clusters which supports gluster should be fetched or have a condition
in the below loop
Line 40:
Line 41: for (VDSGroup cluster : clusters) {
Line 42: List<VDS> upServers =
getClusterUtils().getAllUpServers(cluster.getId());
Line 43:
Line 72: Map<HookKey, GlusterHookEntity> hooksWithConflictChanges = new
HashMap<HookKey,GlusterHookEntity>();
Line 73:
Line 74: for (Pair<VDS, VDSReturnValue> pairResult : pairResults) {
Line 75: VDS server = pairResult.getFirst();
Line 76: List<GlusterHookEntity> returnedHooks =
(List<GlusterHookEntity>) pairResult.getSecond().getReturnValue();
This could be null if the vds command was failed before.
Line 77:
Line 78: for (GlusterHookEntity retHook: returnedHooks) {
Line 79: HookKey key=
HookKey.valueOf(retHook.getGlusterCommand(),retHook.getStage(),retHook.getName());
Line 80: GlusterHookEntity existingHook =
existingHookMap.get(key);
Line 78: for (GlusterHookEntity retHook: returnedHooks) {
Line 79: HookKey key=
HookKey.valueOf(retHook.getGlusterCommand(),retHook.getStage(),retHook.getName());
Line 80: GlusterHookEntity existingHook =
existingHookMap.get(key);
Line 81:
Line 82: addToReturnedHookKeyList(returnedHookKeyList, retHook);
We won't need a separate method for it, we can move
if (!returnedHookKeyList.contains(key))
returnedHookKeyList.add(key);
here and we can use the same HookKey.
The same can be reduced to single line if you prefer Set instead of List.
Line 83:
Line 84: if (existingHook != null) {
Line 85: GlusterServerHook serverHook =
getServerHookForServer(existingHook, server);
Line 86: // if gsh null, insert serverhook
Line 82: addToReturnedHookKeyList(returnedHookKeyList, retHook);
Line 83:
Line 84: if (existingHook != null) {
Line 85: GlusterServerHook serverHook =
getServerHookForServer(existingHook, server);
Line 86: // if gsh null, insert serverhook
gsh?
Line 87: if (serverHook == null) {
Line 88: log.infoFormat("Adding new server hook {0} in
server {1} ", key,server);
Line 89:
newServerHooks.add(buildServerHook(server.getId(), existingHook.getId(),
retHook));
Line 90: } else {
Line 98: }
Line 99:
Line 100: }
Line 101: Integer oldConflictValue =
existingHook.getConflictStatus();
Line 102: existingHook = setConflict(existingHook, retHook);
This can go wrong if there is a different type of conflict in different hosts.
Line 103: if
(!oldConflictValue.equals(existingHook.getConflictStatus())) {
Line 104: log.debugFormat("Conflict change detected for
hook {0} in server {1} ", key,server);
Line 105: if
(hooksWithConflictChanges.containsKey(key)) {
Line 106:
existingHook.setConflictStatus(existingHook.getConflictStatus() |
hooksWithConflictChanges.get(key).getConflictStatus());
Line 105: if
(hooksWithConflictChanges.containsKey(key)) {
Line 106:
existingHook.setConflictStatus(existingHook.getConflictStatus() |
hooksWithConflictChanges.get(key).getConflictStatus());
Line 107: }
Line 108: hooksWithConflictChanges.put(key,
existingHook);
Line 109: }
How about a hook is in Enabled status in db, but Disabled in all the (UP)hosts?
We can change the hook status to Disabled in db instead of marking it as
conflict.
Line 110: } else {
Line 111: log.infoFormat("Adding new hook {0} in server {1}
", key,server);
Line 112: existingHook = newHookMap.get(key);
Line 113: if (existingHook == null) {
Line 116: }
Line 117: GlusterServerHook gsh =
buildServerHook(server.getId(), existingHook.getId(), retHook);
Line 118: existingHook.getServerHooks().add(gsh);
Line 119: existingHook = setConflict(existingHook, retHook);
Line 120: newHookMap.put(key, existingHook);
newHookMap.put(key, existingHook)
can be moved to the above if block
Line 121: }
Line 122: }
Line 123: }
Line 124:
--
To view, visit http://gerrit.ovirt.org/13943
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2766edd4e810677a200cf5c45d334cabef5f2924
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Sahina Bose <sab...@redhat.com>
Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com>
Gerrit-Reviewer: Sahina Bose <sab...@redhat.com>
Gerrit-Reviewer: Shireesh Anjal <san...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
