Allon Mureinik has posted comments on this change.
Change subject: engine: Refresh gluster data periodically
......................................................................
Patch Set 20: (10 inline comments)
Much better!
See some comments inline.
None of them are dealbreakers, IMHO
....................................................
File backend/manager/dbscripts/network_sp.sql
Line 310: AS $procedure$
Line 311: BEGIN
Line 312: RETURN QUERY SELECT i.*
Line 313: FROM vds_interface_view i,
Line 314: vds_static v
Please use spaces and not tabs.
Also, I'd prefer the JOIN syntax, but it's really a matter of taste
Line 315: WHERE i.addr = v_addr
Line 316: AND v.vds_group_id = v_cluster_id
Line 317: AND i.vds_id = v.vds_id;
Line 318:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterManager.java
Line 69: */
Line 70: public class GlusterManager {
Line 71: private static final String ENTITY_BRICK = "brick";
Line 72: private static final String ENTITY_OPTION = "option";
Line 73: private static GlusterManager instance = new GlusterManager();
IMHO, should be final too.
Line 74: private static Log log = LogFactory.getLog(GlusterManager.class);
Line 75: private static final int GLUSTER_REFRESH_RATE_LIGHT =
Line 76: Config.<Integer>
GetValue(ConfigValues.GlusterRefreshRateLight);
Line 77: private static final int GLUSTER_REFRESH_RATE_HEAVY =
Line 70: public class GlusterManager {
Line 71: private static final String ENTITY_BRICK = "brick";
Line 72: private static final String ENTITY_OPTION = "option";
Line 73: private static GlusterManager instance = new GlusterManager();
Line 74: private static Log log = LogFactory.getLog(GlusterManager.class);
should be final
Line 75: private static final int GLUSTER_REFRESH_RATE_LIGHT =
Line 76: Config.<Integer>
GetValue(ConfigValues.GlusterRefreshRateLight);
Line 77: private static final int GLUSTER_REFRESH_RATE_HEAVY =
Line 78: Config.<Integer>
GetValue(ConfigValues.GlusterRefreshRateHeavy);
Line 72: private static final String ENTITY_OPTION = "option";
Line 73: private static GlusterManager instance = new GlusterManager();
Line 74: private static Log log = LogFactory.getLog(GlusterManager.class);
Line 75: private static final int GLUSTER_REFRESH_RATE_LIGHT =
Line 76: Config.<Integer>
GetValue(ConfigValues.GlusterRefreshRateLight);
I'd consider moving these statements into an init block or something of the
sorts in order to facilitate easier testing.
Line 77: private static final int GLUSTER_REFRESH_RATE_HEAVY =
Line 78: Config.<Integer>
GetValue(ConfigValues.GlusterRefreshRateHeavy);
Line 79:
Line 80: private GlusterManager() {
Line 318: * @return
Line 319: */
Line 320: private Set<GlusterServerInfo> fetchServers(VDS upServer,
List<VDS> existingServers) {
Line 321: Set<GlusterServerInfo> fetchedServers = null;
Line 322: while (fetchedServers == null && existingServers.size() > 0) {
consider !existingServes.isEmpty()
Line 323: fetchedServers = fetchServers(upServer);
Line 324: if (fetchedServers == null) {
Line 325: logServerMessage(upServer,
AuditLogType.GLUSTER_SERVERS_LIST_FAILED);
Line 326: // Couldn't fetch servers from the up server. Mark it
as non-operational
Line 409: protected Map<String, GlusterVolumeEntity> fetchVolumes(VDS
upServer) {
Line 410: VDSReturnValue result =
Line 411: Backend.getInstance()
Line 412: .getResourceManager()
Line 413:
.RunVdsCommand(VDSCommandType.GlusterVolumesList,
can't you use runVdsCommand here?
Line 414: new
GlusterVolumesListVDSParameters(upServer.getId(), upServer.getvds_group_id()));
Line 415:
Line 416: return result.getSucceeded() ? (Map<String,
GlusterVolumeEntity>) result.getReturnValue() : null;
Line 417: }
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/gluster/GlusterManagerTest.java
Line 152: private List<Guid> addedBrickIds = new ArrayList<Guid>();
Line 153: private List<GlusterBrickEntity> bricksWithChangedStatus = new
ArrayList<GlusterBrickEntity>();
Line 154:
Line 155: @Before
Line 156: public void setUp() throws Exception {
Why do you need an extra method?
Why not just annotate createObjects() with @Before?
Line 157: createObjects();
Line 158: }
Line 159:
Line 160: private void mockConfig() {
Line 156: public void setUp() throws Exception {
Line 157: createObjects();
Line 158: }
Line 159:
Line 160: private void mockConfig() {
You use this method in all (=both) your tests.
Why not just do it inline in the Rule's definition?
See the last section (" Mocking The Same Config Values for the Entire Test
Suite") in http://wiki.ovirt.org/wiki/MockConfigRule
Line 161: mcr.mockConfigValue(ConfigValues.GlusterRefreshRateLight, 5);
Line 162: mcr.mockConfigValue(ConfigValues.GlusterRefreshRateHeavy,
300);
Line 163: }
Line 164:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/NonOperationalReason.java
Line 13: VERSION_INCOMPATIBLE_WITH_CLUSTER(5),
Line 14: KVM_NOT_RUNNING(6),
Line 15: TIMEOUT_RECOVERING_FROM_CRASH(7),
Line 16: VM_NETWORK_IS_BRIDGELESS(8),
Line 17: GLUSTER_COMMAND_FAILED(9),
Note that this statement end with a "," and the next statement is a ";".
I'm guessing this is a git foobar.
Line 18: ;
Line 19:
Line 20: private final int value;
Line 21:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/VDSCommandType.java
Line 138: GlusterHostRemove("org.ovirt.engine.core.vdsbroker.gluster"),
Line 139: GlusterHostAdd("org.ovirt.engine.core.vdsbroker.gluster"),
Line 140: GlusterServersList("org.ovirt.engine.core.vdsbroker.gluster"),
Line 141:
GetGlusterVolumeAdvancedDetails("org.ovirt.engine.core.vdsbroker.gluster"),
Line 142: GlusterVolumesList("org.ovirt.engine.core.vdsbroker.gluster"),
Note that this statement end with a "," and the next statement is a ";".
Is this intentional?
Line 143: ;
Line 144:
Line 145: String packageName;
Line 146:
--
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: 20
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Shireesh Anjal <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Moti Asayag <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Shireesh Anjal <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
