Shireesh Anjal has posted comments on this change.
Change subject: engine:Sync gluster hooks
......................................................................
Patch Set 4: (22 inline comments)
Comments in-line.
....................................................
File backend/manager/dbscripts/upgrade/pre_upgrade/0000_config.sql
Line 142: select
fn_db_add_config_value('GlusterVolumeOptionGroupVirtValue','virt','general');
Line 143: select
fn_db_add_config_value('GlusterVolumeOptionOwnerUserVirtValue','36','general');
Line 144: select
fn_db_add_config_value('GlusterVolumeOptionOwnerGroupVirtValue','36','general');
Line 145: select fn_db_add_config_value('GlusterRefreshRateHooks', '600',
'general');
Line 146: select fn_db_add_config_value('GlusterHooksEnabled', 'true', '3.3');
I believe the entries need to be ordered by key. So please move these two
entries to appropriate places.
Line 147: select
fn_db_add_config_value('GuestToolsSetupIsoPrefix','RHEV-toolsSetup_','general');
Line 148: select fn_db_add_config_value('HardwareInfoEnabled','false','3.0');
Line 149: select fn_db_add_config_value('HardwareInfoEnabled','false','3.1');
Line 150: select fn_db_add_config_value('HardwareInfoEnabled','true','3.2');
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterHookSyncJob.java
Line 40: log.debug("Refreshing hooks list");
Line 41: List<VDSGroup> clusters = getClusterDao().getAll();
Line 42:
Line 43: for (VDSGroup cluster : clusters) {
Line 44: if (!cluster.supportsGlusterService() &&
GlusterFeatureSupported.glusterHooks(cluster.getcompatibility_version()))
I think this shold be:
if (!(cluster.supportsGlusterService() &&
GlusterFeatureSupported.glusterHooks(cluster.getcompatibility_version())))
I would not mind putting the && condition above (everything except the !) into
a separate method called
private boolean requiresHooksRefresh(VDSGroup cluster)
The if condition will then look like:
if (!requiresHooksRefresh(cluster)) {
...
}
which I think is easier to understand.
Line 45: {
Line 46: break;
Line 47: }
Line 48:
Line 42:
Line 43: for (VDSGroup cluster : clusters) {
Line 44: if (!cluster.supportsGlusterService() &&
GlusterFeatureSupported.glusterHooks(cluster.getcompatibility_version()))
Line 45: {
Line 46: break;
Should this rather be "continue;" ?
Line 47: }
Line 48:
Line 49: List<VDS> upServers =
getClusterUtils().getAllUpServers(cluster.getId());
Line 50:
Line 44: if (!cluster.supportsGlusterService() &&
GlusterFeatureSupported.glusterHooks(cluster.getcompatibility_version()))
Line 45: {
Line 46: break;
Line 47: }
Line 48:
A "debug" message printing name of the cluster whose hooks are being refreshed
could be useful.
Line 49: List<VDS> upServers =
getClusterUtils().getAllUpServers(cluster.getId());
Line 50:
Line 51: List<Callable<Pair<VDS, VDSReturnValue>>> taskList = new
ArrayList<Callable<Pair<VDS, VDSReturnValue>>>();
Line 52: for (final VDS upServer : upServers) {
Line 80:
Line 81: for (Pair<VDS, VDSReturnValue> pairResult : pairResults) {
Line 82: VDS server = pairResult.getFirst();
Line 83: if (!pairResult.getSecond().getSucceeded()) {
Line 84: log.infoFormat("Failed to get hooklist from server {0}
with error {1} ", server, pairResult.getSecond().getVdsError());
* s/hooklist/list of hooks
* There is no toString() on VDSError. So you'll have to format the error
message using getCode() and/or getMessage()
Line 85: logUtil.logServerMessage(server,
AuditLogType.GLUSTER_HOOK_LIST_FAILED);
Line 86: break;
Line 87: }
Line 88: List<GlusterHookEntity> returnedHooks =
(List<GlusterHookEntity>) pairResult.getSecond().getReturnValue();
Line 82: VDS server = pairResult.getFirst();
Line 83: if (!pairResult.getSecond().getSucceeded()) {
Line 84: log.infoFormat("Failed to get hooklist from server {0}
with error {1} ", server, pairResult.getSecond().getVdsError());
Line 85: logUtil.logServerMessage(server,
AuditLogType.GLUSTER_HOOK_LIST_FAILED);
Line 86: break;
Should this rather be "continue;" ?
Line 87: }
Line 88: List<GlusterHookEntity> returnedHooks =
(List<GlusterHookEntity>) pairResult.getSecond().getReturnValue();
Line 89:
Line 90: for (GlusterHookEntity retHook: returnedHooks) {
Line 84: log.infoFormat("Failed to get hooklist from server {0}
with error {1} ", server, pairResult.getSecond().getVdsError());
Line 85: logUtil.logServerMessage(server,
AuditLogType.GLUSTER_HOOK_LIST_FAILED);
Line 86: break;
Line 87: }
Line 88: List<GlusterHookEntity> returnedHooks =
(List<GlusterHookEntity>) pairResult.getSecond().getReturnValue();
Minor - I would call this "fetchedHooks" :)
Line 89:
Line 90: for (GlusterHookEntity retHook: returnedHooks) {
Line 91: HookKey key=
HookKey.valueOf(retHook.getGlusterCommand(),retHook.getStage(),retHook.getName());
Line 92: GlusterHookEntity existingHook =
existingHookMap.get(key);
Line 87: }
Line 88: List<GlusterHookEntity> returnedHooks =
(List<GlusterHookEntity>) pairResult.getSecond().getReturnValue();
Line 89:
Line 90: for (GlusterHookEntity retHook: returnedHooks) {
Line 91: HookKey key=
HookKey.valueOf(retHook.getGlusterCommand(),retHook.getStage(),retHook.getName());
If the purpose of HookKey class is to represent a hook key which is combination
of command, stage and name, so that it can be used as key in the maps, I think
it can be done in a simpler way as follows:
Introduce two methods in GlusterHookEntity:
1) public String getHookKey();
which will return a string concatenation of the command, stage and name
2) public boolean representsSameHook(GlusterHookEntity other);
which will return true if getHookKey() returns same string.
Line 92: GlusterHookEntity existingHook =
existingHookMap.get(key);
Line 93:
Line 94: addToReturnedHookKeyList(returnedHookKeyList, retHook);
Line 95:
Line 90: for (GlusterHookEntity retHook: returnedHooks) {
Line 91: HookKey key=
HookKey.valueOf(retHook.getGlusterCommand(),retHook.getStage(),retHook.getName());
Line 92: GlusterHookEntity existingHook =
existingHookMap.get(key);
Line 93:
Line 94: addToReturnedHookKeyList(returnedHookKeyList, retHook);
If you use a Set instead of list, the method addToReturnedHookKeyList() can be
replaced by a single statement like:
fetchedHookKeys.add(fetchedHook.getHookKey());
Line 95:
Line 96: if (existingHook != null) {
Line 97: GlusterServerHook serverHook =
getServerHookForServer(existingHook, server);
Line 98: // if null, insert serverhook
Line 97: GlusterServerHook serverHook =
getServerHookForServer(existingHook, server);
Line 98: // if null, insert serverhook
Line 99: if (serverHook == null) {
Line 100: log.infoFormat("Adding new server hook {0} in
server {1} ", key,server);
Line 101:
newServerHooks.add(buildServerHook(server.getId(), existingHook.getId(),
retHook));
I thought we're supposed to insert into ServerHooks only in case of conflicts?
Line 102: } else {
Line 103: // if not null, compare to see if update required
Line 104: if
(!(serverHook.getChecksum().equals(retHook.getChecksum()) &&
serverHook.getContentType().equals(retHook.getContentType())
Line 105: &&
serverHook.getStatus().equals(retHook.getStatus()))) {
Line 99: if (serverHook == null) {
Line 100: log.infoFormat("Adding new server hook {0} in
server {1} ", key,server);
Line 101:
newServerHooks.add(buildServerHook(server.getId(), existingHook.getId(),
retHook));
Line 102: } else {
Line 103: // if not null, compare to see if update required
Minor - fix comment alignment
Line 104: if
(!(serverHook.getChecksum().equals(retHook.getChecksum()) &&
serverHook.getContentType().equals(retHook.getContentType())
Line 105: &&
serverHook.getStatus().equals(retHook.getStatus()))) {
Line 106:
serverHook.setChecksum(retHook.getChecksum());
Line 107:
serverHook.setContentType(retHook.getContentType());
Line 100: log.infoFormat("Adding new server hook {0} in
server {1} ", key,server);
Line 101:
newServerHooks.add(buildServerHook(server.getId(), existingHook.getId(),
retHook));
Line 102: } else {
Line 103: // if not null, compare to see if update required
Line 104: if
(!(serverHook.getChecksum().equals(retHook.getChecksum()) &&
serverHook.getContentType().equals(retHook.getContentType())
Will it be nicer if we have methods in GlusterServerHook for this. Something
like:
if(!serverHook.isInSyncWith(fetchedHook)) {
serverHook.syncWith(fetchedHook);
...
}
Line 105: &&
serverHook.getStatus().equals(retHook.getStatus()))) {
Line 106:
serverHook.setChecksum(retHook.getChecksum());
Line 107:
serverHook.setContentType(retHook.getContentType());
Line 108: serverHook.setStatus(retHook.getStatus());
Line 107:
serverHook.setContentType(retHook.getContentType());
Line 108: serverHook.setStatus(retHook.getStatus());
Line 109: updatedServerHooks.add(serverHook);
Line 110: }
Line 111:
Minor - please remove empty line.
Line 112: }
Line 113: Integer oldConflictValue =
existingHook.getConflictStatus();
Line 114: existingHook = setConflict(existingHook, retHook);
Line 115: if
(!oldConflictValue.equals(existingHook.getConflictStatus())) {
Line 120: hooksWithConflictChanges.put(key,
existingHook);
Line 121: }
Line 122: } else {
Line 123: log.infoFormat("Adding new hook {0} in server {1}
", key,server);
Line 124: existingHook = newHookMap.get(key);
While this code works, I think creating a new variable with name newHook will
be more readable.
Line 125: if (existingHook == null) {
Line 126: existingHook = retHook;
Line 127: existingHook.setId(Guid.NewGuid());
Line 128: }
Line 185: }
Line 186: if (!hook.getStatus().equals(returnedHook.getStatus())) {
Line 187: hook.addStatusConflict();
Line 188: }
Line 189: return hook;
I don't think it's a good idea to return an object that you're accepting as a
method parameter. There are two options:
1) Make the method void and don't return anything
2) Rename the method as getConflictCode(), return the conflict code (int) from
the method, and set it on the hook outside this method.
I prefer option 2.
Line 190: }
Line 191:
Line 192: private GlusterServerHook buildServerHook(Guid serverId, Guid
hookId, GlusterHookEntity returnedHook) {
Line 193: GlusterServerHook serverHook = new GlusterServerHook();
Line 200: }
Line 201:
Line 202: private GlusterServerHook
getServerHookForServer(GlusterHookEntity hook, VDS server) {
Line 203: if (hook.getServerHooks() == null) {
Line 204:
hook.setServerHooks(getHooksDao().getGlusterServerHooks(hook.getId()));
I think the whole method can be a single line:
return getHooksDao().getGlusterServerHook(hook.getId(), server.getId());
Line 205: }
Line 206: for (GlusterServerHook serverHook: hook.getServerHooks()) {
Line 207: if (serverHook.getServerId().equals(server.getId())) {
Line 208: return serverHook;
Line 202: private GlusterServerHook
getServerHookForServer(GlusterHookEntity hook, VDS server) {
Line 203: if (hook.getServerHooks() == null) {
Line 204:
hook.setServerHooks(getHooksDao().getGlusterServerHooks(hook.getId()));
Line 205: }
Line 206: for (GlusterServerHook serverHook: hook.getServerHooks()) {
hook.getServerHooks() might still return null (if there are no server-hooks in
db), so this is a potential NPE. This is a moot point if you change the method
as suggested above.
Line 207: if (serverHook.getServerId().equals(server.getId())) {
Line 208: return serverHook;
Line 209: }
Line 210: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterJobsManager.java
Line 44: "refreshHooks",
Line 45: new Class[0],
Line 46: new Object[0],
Line 47: getGlusterRefreshRateHooks(),
Line 48: getGlusterRefreshRateHooks(),
I know this is existing convention in the class that you've followed, but we
can probably refactor this to have a single method like:
private static int getRefreshRate(ConfigValues configName);
instead of one method each for light, heavy and hooks.
I request you to do this, only if you want to :)
Line 49: TimeUnit.SECONDS);
Line 50:
Line 51: }
Line 52:
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/gluster/GlusterHookSyncJobTest.java
Line 157: return cluster;
Line 158: }
Line 159:
Line 160: @Test
Line 161: public void syncHooks() {
I guess more tests are required to test various sync scenarios and conflict
conditions
Line 162: initMocks();
Line 163: hookManager.refreshHooks();
Line 164: Mockito.verify(hooksDao,
times(1)).save(any(GlusterHookEntity.class));
Line 165: //Mockito.verify(hooksDao,
times(1)).saveGlusterServerHook(any(GlusterServerHook.class));
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/ConfigValues.java
Line 1358:
Line 1359:
Line 1360: @TypeConverterAttribute(Integer.class)
Line 1361: @DefaultValueAttribute("600")
Line 1362: GlusterRefreshRateHooks(503),
Let's have these with code 424 and 425, in the "gluster" range.
Line 1363:
Line 1364: @TypeConverterAttribute(Boolean.class)
Line 1365: @DefaultValueAttribute("false")
Line 1366: GlusterHooksEnabled(504),
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/auditloghandling/AuditLogDirector.java
Line 106: severities.put(AuditLogType.GLUSTER_HOOK_ENABLE_FAILED,
AuditLogSeverity.ERROR);
Line 107: severities.put(AuditLogType.GLUSTER_HOOK_DISABLE,
AuditLogSeverity.NORMAL);
Line 108: severities.put(AuditLogType.GLUSTER_HOOK_DISABLE_PARTIAL,
AuditLogSeverity.WARNING);
Line 109: severities.put(AuditLogType.GLUSTER_HOOK_DISABLE_FAILED,
AuditLogSeverity.ERROR);
Line 110: severities.put(AuditLogType.GLUSTER_HOOK_LIST_FAILED,
AuditLogSeverity.ERROR);
I think we should also generate "WARNING" audit messages whenever we detect new
conflicts.
Line 111: }
Line 112:
Line 113: private static void initDefaultSeverities() {
Line 114: severities.put(AuditLogType.UNASSIGNED,
AuditLogSeverity.NORMAL);
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/gluster/GlusterHooksDaoDbFacadeImpl.java
Line 74: .addValue("includeContent", false));
Line 75: return glusterHook;
Line 76: }
Line 77:
Line 78: public List<GlusterServerHook> getGlusterServerHooks(Guid hookId) {
Need to add a new test case for this in the DAO test class.
Line 79: List<GlusterServerHook> serverHooks =
getCallsHandler().executeReadList("GetGlusterServerHooksById",
glusterServerHookRowMapper,
Line 80: createIdParameterMapper(hookId));
Line 81: return serverHooks;
Line 82: }
--
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: 4
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
