Shireesh Anjal has posted comments on this change.
Change subject: engine: Fix SHD service not displaying issue (#885592)
......................................................................
Patch Set 1: (8 inline comments)
....................................................
File backend/manager/dbscripts/gluster_volumes_sp.sql
Line 105: v_option_val VARCHAR(8192),
Line 106: v_vol_type VARCHAR(32))
Line 107: RETURNS SETOF gluster_volumes
Line 108: AS $procedure$
Line 109: BEGIN
Can you please add some indentation so that it is easier to review?
Line 110: RETURN QUERY SELECT *
Line 111: FROM gluster_volumes
Line 112: WHERE cluster_id = v_cluster_id AND status = v_status AND vol_type =
v_vol_type
Line 113: AND id IN (SELECT volume_id FROM gluster_volume_options
Line 114: WHERE option_key=v_option_key AND option_val=v_option_val);
Line 115: END; $procedure$
Line 116: LANGUAGE plpgsql;
Line 117:
Line 118: Create or replace FUNCTION GetGlusterVolumesByVolumeType(v_cluster_id
UUID,
Instead of calling this SP two times for REPLICATE/DISTRIBUTED_REPLICATE, you
can have it take a comma separated list of volume types, and use the "IN"
clause to check for either. Check DeleteGlusterVolumesByGuids() for something
similar
Line 119: v_status VARCHAR(32),
Line 120: v_vol_type VARCHAR(32))
Line 121: RETURNS SETOF gluster_volumes
Line 122: AS $procedure$
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GetGlusterVolumeAdvancedDetailsQuery.java
Line 21: */
Line 22: public class GetGlusterVolumeAdvancedDetailsQuery<P extends
GlusterVolumeAdvancedDetailsParameters> extends GlusterQueriesCommandBase<P> {
Line 23:
Line 24: private static final String OPTION_KEY = "nfs.disable";
Line 25: private static final String OPTION_VALUE = "off";
It'll be easier to read the usage of these constants if they are renamed to
something like
- OPTION_KEY_NFS_DISABLE
- OPTION_VALUE_OFF
Line 26:
Line 27: public GetGlusterVolumeAdvancedDetailsQuery(P params) {
Line 28: super(params);
Line 29: }
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/gluster/GlusterVolumeDao.java
Line 34: public List<GlusterVolumeEntity>
getVolumesByOptionAndVolumeType(Guid clusterId,
Line 35: GlusterStatus status,
Line 36: String optionKey,
Line 37: String optionValue,
Line 38: GlusterVolumeType volumeType);
Since it takes status also, I think this method should be called
getVolumesByStatusTypeAndOption(). If you choose this name, it would be nicer
if volume type parameter comes before the option parameters.
Also, as suggested in a previous comment, this should take
List<GlusterVolumeType> so that volumes can be fetched by
REPLICATE/DISTRIBUTED_REPLICATE in one go.
Line 39:
Line 40: public List<GlusterVolumeEntity> getVolumesByVolumeType(Guid
clusterId,
Line 41: GlusterStatus status,
Line 42: GlusterVolumeType volumeType);
Line 38: GlusterVolumeType volumeType);
Line 39:
Line 40: public List<GlusterVolumeEntity> getVolumesByVolumeType(Guid
clusterId,
Line 41: GlusterStatus status,
Line 42: GlusterVolumeType volumeType);
Since it takes the status also, I think this method should be called
getVolumesByStatusAndType()
Also, as suggested in a previous comment, this should take
List<GlusterVolumeType> so that volumes can be fetched by
REPLICATE/DISTRIBUTED_REPLICATE in one go.
Line 43:
Line 44: @Override
Line 45: public List<GlusterVolumeEntity> getAllWithQuery(String query);
Line 46:
....................................................
File
backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/gluster/GlusterVolumeDaoTest.java
Line 31: private static final Guid EXISTING_VOL_REPL_ID = new
Guid("b2cb2f73-fab3-4a42-93f0-d5e4c069a43e");
Line 32: private static final String EXISTING_VOL_REPL_NAME =
"test-vol-replicate-1";
Line 33: private static final String NEW_VOL_NAME = "test-new-vol-1";
Line 34: private static final String NFS_OPTION_KEY = "nfs.disable";
Line 35: private static final String NFS_OPTION_VALUE = "off";
I suggest to use
- OPTION_KEY_NFS_DISABLE
- OPTION_VALUE_OFF
Line 36: private GlusterVolumeDao dao;
Line 37: private VdsStatic server;
Line 38: private GlusterVolumeEntity existingDistVol;
Line 39: private GlusterVolumeEntity existingReplVol;
Line 315: GlusterVolumeType.DISTRIBUTED_REPLICATE);
Line 316:
Line 317: assertTrue(volumes != null);
Line 318: assertTrue(volumes.contains(existingReplVol));
Line 319: assertTrue(volumes.get(0).isNfsEnabled()
Maybe this assertion can be done in a loop for all returned volumes?
Line 320: && volumes.get(0).getVolumeType() ==
GlusterVolumeType.DISTRIBUTED_REPLICATE);
Line 321: }
Line 322:
Line 323: @Test
Line 326: dao.getVolumesByVolumeType(CLUSTER_ID,
GlusterStatus.UP, GlusterVolumeType.DISTRIBUTE);
Line 327:
Line 328: assertTrue(volumes != null);
Line 329: assertTrue(volumes.contains(existingDistVol));
Line 330: assertTrue(volumes.get(0).getVolumeType() ==
GlusterVolumeType.DISTRIBUTE);
Maybe this assertion can be done in a loop for all returned volumes?
Line 331: }
Line 332:
Line 333: @Test
Line 334: public void testRemoveTransportTypes() {
--
To view, visit http://gerrit.ovirt.org/10336
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id11725f44ab3fdd36f76fe569d7610a411518ee1
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Dhandapani Gopal <[email protected]>
Gerrit-Reviewer: Eli Mesika <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Shireesh Anjal <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
