Shireesh Anjal has posted comments on this change.
Change subject: engine: Manage Gluster hooks support
......................................................................
Patch Set 20: (15 inline comments)
....................................................
File backend/manager/dbscripts/gluster_hooks_sp.sql
Line 171:
Line 172: Create or replace FUNCTION UpdateGlusterHook(v_id UUID,
Line 173: v_hook_status
VARCHAR(50),
Line 174: v_content_type
VARCHAR(50),
Line 175: v_checksum VARCHAR(256),
Will there be cases where we update the checksum without updating the content?
Line 176: v_conflict_status
INTEGER)
Line 177: RETURNS VOID
Line 178: AS $procedure$
Line 179: BEGIN
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/gluster/GlusterHookConflictFlags.java
Line 2:
Line 3: public enum GlusterHookConflictFlags {
Line 4: CONTENT_CONFLICT (1), //0b001
Line 5: STATUS_CONFLICT(2), //0b010
Line 6: MISSING_HOOK (4); //0b100
Do you also need to add CONTENT_AND_STATUS_CONFLICT(3) ?
Also, if we don't envisage any other kinds of conflicts, maybe we can just
treat these as an enum with 4 values, instead of thinking of them as bit
values, for the sake of simplicity.
Line 7:
Line 8: private Integer flag;
Line 9:
Line 10: private GlusterHookConflictFlags(Integer value) {
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/gluster/GlusterHookEntity.java
Line 38: private String content;
Line 39:
Line 40: private Integer conflictStatus;
Line 41:
Line 42: private List<GlusterServerHook> serverHooks;
How about initializing this with an empty list?
Line 43:
Line 44: @Override
Line 45: public Guid getId() {
Line 46: return getId(true);
Line 46: return getId(true);
Line 47: }
Line 48:
Line 49: public Guid getId(boolean generateIfNull) {
Line 50: if (id == null && generateIfNull) {
Though the chances of multiple threads calling getId() on the same object seem
less, it might still be a good idea to synchronize this part.
Line 51: id = Guid.NewGuid();
Line 52: }
Line 53: return id;
Line 54: }
Line 182: && name.equals(hook.getName())
Line 183: && contentType == hook.getContentType()
Line 184: && conflictStatus == hook.getConflictStatus()
Line 185: && checksum.equals(hook.getChecksum())
Line 186: && status == hook.getStatus())) {
For string variables like name, checksum, etc., please consider using
ObjectUtils.objectsEqual(), which handles cases where one or both of the
objects are null.
Line 187: return false;
Line 188: }
Line 189: return true;
Line 190: }
Line 205: public boolean isContentConflict() {
Line 206: return (this.conflictStatus & CONTENT_CONFLICT.getValue()) ==
CONTENT_CONFLICT.getValue();
Line 207: }
Line 208:
Line 209: public void setConflictValue(boolean missingHook, boolean
statusConflict, boolean contentConflict) {
Since this can potentially change the existing value of conflictStatus, should
we change the method name to something like addConflict() ? Currently it sounds
like a setter, which it isn't.
Line 210: this.conflictStatus = 0;
Line 211: if (missingHook) {
Line 212: this.conflictStatus = this.conflictStatus |
MISSING_HOOK.getValue();
Line 213: }
Line 219: }
Line 220: }
Line 221:
Line 222: public boolean hasConflicts() {
Line 223: return this.conflictStatus > 0;
I guess we need a null check as well.
Line 224: }
Line 225:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/gluster/GlusterServerHook.java
Line 42: this.status = status;
Line 43: }
Line 44:
Line 45: public void setStatus(String status) {
Line 46: this.status = GlusterHookStatus.valueOf(status);
Does this also require a null check similar to setcontentType() ?
Line 47: }
Line 48:
Line 49: public GlusterHookContentType getContentType() {
Line 50: return contentType;
Line 56:
Line 57: public void setContentType(String contentType) {
Line 58: if (contentType != null) {
Line 59: this.contentType =
GlusterHookContentType.valueOf(contentType);
Line 60: }
Should one of the following happen in case contentType passed is null?
- throw an exception, or
- set this.contentType = null
Line 61: }
Line 62:
Line 63: public String getChecksum() {
Line 64: return checksum;
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/gluster/GlusterParameters.java
Line 6: import org.ovirt.engine.core.compat.Guid;
Line 7:
Line 8: /**
Line 9: * Parameter class with cluster Id as parameters. <br>
Line 10: * This will be used by gluster volume option help command and Gluster
Hooks query. <br>
Maybe we can just remove this line altogether from the comment. No point in
keeping modifying it every time it is used :)
Line 11: */
Line 12: public class GlusterParameters extends VdcQueryParametersBase {
Line 13: private static final long serialVersionUID = -1224829720081853632L;
Line 14:
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/DbFacade.java
Line 869: * Returns the singleton instance of {@link GlusterHooksDao}.
Line 870: *
Line 871: * @return the dao
Line 872: */
Line 873: public GlusterHooksDao getGlusterHookDao() {
Since the dao interface is called GlusterHook*s*Dao, should the method also be
called getGlusterHook*s*Dao() ?
Line 874: return getDao(GlusterHooksDao.class);
Line 875: }
Line 876:
Line 877: public void setOnStartConnectionTimeout(int
onStartConnectionTimeout) {
....................................................
File
backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/gluster/GlusterHooksDaoTest.java
Line 100: hookIds.add(FixturesTool.HOOK_ID2);
Line 101: dao.removeAll(hookIds);
Line 102: List<GlusterHookEntity> hooks =
dao.getByClusterId(FixturesTool.CLUSTER_ID);
Line 103: assertNotNull(hooks);
Line 104: assertTrue(hooks.isEmpty());
It will be good to test removeAll() with the case where multiple entries are
removed, but doesn't empty the table. Something like Add 3 hooks, remove 2, and
test that 1 remains in the DB.
I remember a case where someone changed a SP (related to fnSplitter), resulting
in a bug wherein it started deleting all the records instead of only the ones
passed as argument.
Since our DAO test case always expected the table to be empty after the call to
removeAll(), the bug was not caught in build :)
Line 105: }
Line 106:
Line 107: @Test
Line 108: public void testUpdateGlusterHookStatus() {
Line 113: }
Line 114:
Line 115: @Test
Line 116: public void testUpdateGlusterServerHookStatus() {
Line 117: dao.updateGlusterServerHookStatus(FixturesTool.HOOK_ID,
SERVER_ID, GlusterHookStatus.ENABLED);
The status of the pre-defined hook in fixtures.xml is already ENABLED. So maybe
you should update it to DISABLED and verify the same.
What I do to make such tests easier to read is, first fetch the current status
and verify (assert) it, then update it to a different value, fetch again and
verify the new value.
Line 118: GlusterServerHook serverhook =
dao.getGlusterServerHook(FixturesTool.HOOK_ID, SERVER_ID);
Line 119: assertNotNull(serverhook);
Line 120: assertEquals(GlusterHookStatus.ENABLED,
serverhook.getStatus());
Line 121: }
Line 139:
Line 140: @Test
Line 141: public void updateGlusterHookContent() {
Line 142: String updateContent = "Updated script content to test";
Line 143: String updateChecksum = "ddffeef712fc008f857e77a2f3f179c710";
You can re-use the already defined CHECKSUM constant here.
Line 144: dao.updateGlusterHookContent(FixturesTool.HOOK_ID,
updateChecksum, updateContent);
Line 145: GlusterHookEntity hook =
dao.getById(FixturesTool.HOOK_ID,true);
Line 146: assertNotNull(hook);
Line 147: assertEquals(updateContent, hook.getContent());
Line 152: public void testUpdateGlusterHook() {
Line 153: dao.updateGlusterHook(getGlusterHook());
Line 154: GlusterHookEntity hook = dao.getById(FixturesTool.HOOK_ID);
Line 155: assertNotNull(hook);
Line 156: assertEquals(CHECKSUM, hook.getChecksum());
Maybe we can compare the whole object instead of just checksum?
Line 157: }
Line 158:
--
To view, visit http://gerrit.ovirt.org/10650
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If8b2d261ead720fccea6884acbc806ec79e52b36
Gerrit-PatchSet: 20
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Selvasundaram <[email protected]>
Gerrit-Reviewer: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Kanagaraj M <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Sahina Bose <[email protected]>
Gerrit-Reviewer: Selvasundaram <[email protected]>
Gerrit-Reviewer: Shireesh Anjal <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
