Martin Peřina has posted comments on this change.
Change subject: core: Introducing VdsValidator class
......................................................................
Patch Set 1: (5 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsNotRespondingTreatmentCommand.java
Line 50: setVds(null);
Line 51: if (getVds() == null) {
Line 52: setCommandShouldBeLogged(false);
Line 53: log.infoFormat("Host {0}({1}) not fenced since it doesn't
exist anymore.", getVdsName(), getVdsId());
Line 54: return;
Well, this hasn't been called in previous version of the command. Wouldn't this
change the behavior of the command?
Line 55: }
Line 56:
Line 57: validator = new VdsValidator(getVds());
Line 58: if (validator.shouldVdsBeFenced()) {
Line 54: return;
Line 55: }
Line 56:
Line 57: validator = new VdsValidator(getVds());
Line 58: if (validator.shouldVdsBeFenced()) {
Again the test is here because it was here in previous version of the command.
I also think it's safer to check it just before command execution.
Line 59: super.executeCommand();
Line 60: } else {
Line 61: setCommandShouldBeLogged(false);
Line 62: log.infoFormat("Host {0}({1}) not fenced since it's status
is ok, or it doesn't exist anymore.",
Line 60: } else {
Line 61: setCommandShouldBeLogged(false);
Line 62: log.infoFormat("Host {0}({1}) not fenced since it's status
is ok, or it doesn't exist anymore.",
Line 63: getVdsName(), getVdsId());
Line 64: }
Same comment as above in "getVds() == null" test
Line 65: }
Line 66:
Line 67: @Override
Line 68: protected void setStatus() {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsValidator.java
Line 4:
Line 5: /**
Line 6: * Contains methods used to validate VDS status to execute some
operation on it
Line 7: */
Line 8: public class VdsValidator {
Well I created it the same way as StoragePoolValidator. And in future this
class will have much more methods ...
Line 9: /**
Line 10: * Vds instance
Line 11: */
Line 12: private VDS vds;
Line 4:
Line 5: /**
Line 6: * Contains methods used to validate VDS status to execute some
operation on it
Line 7: */
Line 8: public class VdsValidator {
About static/instance method, every solution has its pros and cons. If methods
are static, you have to pass VDS as an argument and you have to check for null
in every methods.
If methods are instance, VDS instance is passed inside constructor and checked
for null only there. But in each command using this class you have to create
new instance of it.
As I wrote above I used the same approach as in StoragePoolValidator ...
Line 9: /**
Line 10: * Vds instance
Line 11: */
Line 12: private VDS vds;
--
To view, visit http://gerrit.ovirt.org/16319
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6f8d22a148513bdbfa46b195e92bc90d66a962ba
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Martin Peřina <[email protected]>
Gerrit-Reviewer: Eli Mesika <[email protected]>
Gerrit-Reviewer: Martin Peřina <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: Yaniv Bronhaim <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
