Allon Mureinik has posted comments on this change.
Change subject: core : WIP : added ReconstructMasterDomain to
InitVdsOnUpCommand (#840838)
......................................................................
Patch Set 7: I would prefer that you didn't submit this
(11 inline comments)
See inline.
Also, on a general note: you have a fairly complicated state-machine here. I'd
like to see a unit test for it, especially considering the transition from one
state to another can relatively easily be mocked away.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/InitVdsOnUpCommand.java
Line 119: || suppressCheck) {
Do you want to connect the host to the storage servers even if
suppressCheck=true?
If so - I'd do the call outside of the if, for readability.
If not - move it after the ||, to allow short circuiting.
Line 126: if (!returnValue) {
I'd declare another boolean in addition to "returnValue", with a better name,
to clarify your logic here.
I find it kinda hard to follow as it is right now.
Line 130: returnValue = _connectPoolSucceeded =
executeConnectStoragePoolCommand(storagePoolGuid);
Separate this to two distinct assignments.
Line 137: // reconstruct the master domain as well.
I'd do this as part of this patch, not leave it to some future one.
Line 154: private void reloadStoragePool() {
why reload?
Just nullify it, and leave the lay getter to init it when (read: if) needed.
Line 159: public boolean reconstructMasterDomain(Guid storagePoolId) {
why public?
Line 167: .getInstance()
use getBackend() instead of Backend.getInstance()
Line 172: } catch (RuntimeException exp) {
Can't we catch a nicer exception?
Line 179: public boolean executeConnectStoragePoolCommand(Guid
storagePoolId) {
why public?
Line 184: .RunVdsCommand(
replace the backedn call with runVdsCommand
Line 191: } catch (RuntimeException exp) {
can't we catch a nicer exception?
--
To view, visit http://gerrit.ovirt.org/7061
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iec43f368370da88f81dbe211331649a16c53f053
Gerrit-PatchSet: 7
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
