Allon Mureinik has posted comments on this change.

Change subject: engine: Refresh gluster data periodically
......................................................................


Patch Set 20: (2 inline comments)

patchset 20: answered questions inline

....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterManager.java
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);
You're initializing a static variable with Config.GetValue.
This means that during the class loading, Config's implementation (which, by 
default, goes to the database) will be called.
This makes the class a bit hard to test - i.e., you have a sort of a race 
between the classloading of MockConfigRule and this class.
If MockConfigRule is called first, you're fine. If not - you have a problem.

If you replace this variable with a method, you will no longer have to worry 
about calling database-related code during class loading.
private int getGlusterRefreshRateLight() {
   return  Config.<Integer> GetValue(ConfigValues.GlusterRefreshRateLight);
}

Moreover, in the current implementation, you're disabling the ability to change 
this config value in runtime - it will be loaded and stored once.
Line 77:     private static final int GLUSTER_REFRESH_RATE_HEAVY =
Line 78:             Config.<Integer> 
GetValue(ConfigValues.GlusterRefreshRateHeavy);
Line 79: 
Line 80:     private GlusterManager() {


....................................................
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),
Missed that, good point.
Thanks for sharing.
Line 18:     ;
Line 19: 
Line 20:     private final int value;
Line 21: 


--
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 <san...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Moti Asayag <masa...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Shireesh Anjal <san...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to