Liron Ar has posted comments on this change. Change subject: core: adding option to not connect host to inactive domains servers ......................................................................
Patch Set 1: (5 comments) http://gerrit.ovirt.org/#/c/27522/1//COMMIT_MSG Commit Message: Line 3: AuthorDate: 2014-05-07 16:26:17 +0300 Line 4: Commit: Liron Aravot <[email protected]> Line 5: CommitDate: 2014-05-08 16:59:44 +0300 Line 6: Line 7: core: adding option to not connect host to inactive domains servers > It seems that you adding an option to *connect* host to inactive domains se currently host always connects to all domains (including inactive one), this patch adds the abillity to *not* connect to those. Line 8: Line 9: This patch adds support for connecting a host only the the storage Line 10: servers of unknown/active domains. Line 11: http://gerrit.ovirt.org/#/c/27522/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ConnectHostToStoragePoolServerCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ConnectHostToStoragePoolServerCommandBase.java: Line 34: statuses = Line 35: EnumSet.of(StorageDomainStatus.Active, StorageDomainStatus.InActive, StorageDomainStatus.Unknown); Line 36: } else { Line 37: statuses = EnumSet.of(StorageDomainStatus.Active, StorageDomainStatus.Unknown); Line 38: } > I would consider to re-factor this to a separate method this code is place in a method called "initConnectionList" that should take care of the connection list initialization, i think that at least for now we shouldn't add another one. Line 39: Line 40: _connections = Line 41: DbFacade.getInstance() Line 42: .getStorageServerConnectionDao() http://gerrit.ovirt.org/#/c/27522/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/DisconnectHostFromStoragePoolServersCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/DisconnectHostFromStoragePoolServersCommand.java: Line 15: import org.ovirt.engine.core.common.vdscommands.VDSReturnValue; Line 16: Line 17: @InternalCommandAttribute Line 18: @NonTransactiveCommandAttribute Line 19: public class DisconnectHostFromStoragePoolServersCommand extends > Why not using the new parameter class also here? There's no need to, this patch isn't about changing the disconnect command that always attempts to disconnect from all the storage connections. this usecase is currently related only to the connect command. Line 20: ConnectHostToStoragePoolServerCommandBase<HostStoragePoolParametersBase> { Line 21: Line 22: public DisconnectHostFromStoragePoolServersCommand(HostStoragePoolParametersBase parameters) { Line 23: super(parameters); Line 26: } Line 27: Line 28: @Override Line 29: protected void executeCommand() { Line 30: initConnectionList(true); > Why not passing the parameter value the parameter is currently relevant only for the connect operation, in the disconnect scenario we always disconnect from the connections of all the domains (including inactive ones) and in this patch i don't intend to change that behavior. Line 31: Line 32: for (Map.Entry<StorageType, List<StorageServerConnections>> connectionToType : getConnectionsTypeMap().entrySet()) { Line 33: disconnectStorageByType(connectionToType.getKey(), connectionToType.getValue()); Line 34: } http://gerrit.ovirt.org/#/c/27522/1/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/ConnectHostToStoragePoolServersParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/ConnectHostToStoragePoolServersParameters.java: Line 3: import org.ovirt.engine.core.common.businessentities.StoragePool; Line 4: import org.ovirt.engine.core.common.businessentities.VDS; Line 5: Line 6: public class ConnectHostToStoragePoolServersParameters extends HostStoragePoolParametersBase { Line 7: private boolean connectToInactiveDomains = true; > I'm surprised this doesn't break checkstyle. Done Line 8: Line 9: public ConnectHostToStoragePoolServersParameters(VDS vds, boolean connectToInactiveDomains) { Line 10: super(vds); Line 11: this.connectToInactiveDomains = connectToInactiveDomains; -- To view, visit http://gerrit.ovirt.org/27522 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7cdde23f8513afa119af6eeff0a02e77c43395f3 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liron Ar <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Tal Nisan <[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
