Allon Mureinik has posted comments on this change. Change subject: core: simplify connectAndRefreshAllUpHosts ......................................................................
Patch Set 2: Code-Review+1 (3 comments) http://gerrit.ovirt.org/#/c/28335/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ReconstructMasterDomainCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ReconstructMasterDomainCommand.java: Line 219: } Line 220: Line 221: private void connectAndRefreshAllUpHosts(final boolean commandSucceeded) { Line 222: if (_isLastMaster || !commandSucceeded) { Line 223: log.warnFormat("skipping connect and refresh for all hosts, last master: {0}, command status: {1}", shouldn't his be log.infoFormat? Line 224: _isLastMaster, commandSucceeded); Line 225: return; Line 226: } Line 227: Line 222: if (_isLastMaster || !commandSucceeded) { Line 223: log.warnFormat("skipping connect and refresh for all hosts, last master: {0}, command status: {1}", Line 224: _isLastMaster, commandSucceeded); Line 225: return; Line 226: } This entire block looks odd in a method called connectAndRefreshAllUpHosts. How about having another method, connectAndRefreshAllUpHostsIfNeeded, which would do something like this: private void connectAndRefreshAllUpHostsIfNeeded(final boolean commandSucceeded) { if (_isLastMaster || !commandSucceeded) { log.infoFormat("skipping connect and refresh for all hosts, last master: {0}, command status: {1}", isLastMaster, commandSucceeded); return; } connectAndRefreshAllUpHosts(); } Line 227: Line 228: List<Callable<Void>> tasks = new ArrayList<Callable<Void>>(); Line 229: for (final VDS vds : getAllRunningVdssInPool()) { Line 230: tasks.add(new Callable<Void>() { Line 231: @Override Line 232: public Void call() throws Exception { Line 233: try { Line 234: if (!connectVdsToNewMaster(vds)) { Line 235: log.warnFormat(""); How about "can't connect ${vdsName} to ${masterDomainName}" ? (peusdocode, of course) Line 236: return null; Line 237: } Line 238: Line 239: List<StoragePoolIsoMap> storagePoolIsoMap = getStoragePoolIsoMapDAO() -- To view, visit http://gerrit.ovirt.org/28335 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I75254ef8fda6732037c261d04d91f23a7148ff8a Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
