Alissa Bonas has posted comments on this change.
Change subject: core: add ability edit NFS path in webadmin
......................................................................
Patch Set 11: (6 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStorageServerConnectionCommand.java
Line 62: return
failCanDoAction(VdcBllMessages.VDS_EMPTY_NAME_OR_ID);
Line 63: }
Line 64:
Line 65: Guid storagePoolId = getParameters().getStoragePoolId();
Line 66: if(storagePoolId == null || storagePoolId.equals(Guid.Empty) )
{
I'll remove it here, but so you know - there's a problem - if someone sets it
to null (for example from REST), it fails with NPE in StorageComandBase. but
that can be fixed in a different patch.
Line 67: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_POOL_NOT_EXIST);
Line 68: }
Line 69:
Line 70: // Check if connection exists by id - otherwise there's
nothing to update
Line 112: return super.canDoAction();
Line 113: }
Line 114:
Line 115:
Line 116: protected String createDomainNamesList(List<StorageDomain>
domains) {
StringUtils is in frontend module, it's not right to use it here. Anyway, I
prefer keeping it as is.
Line 117: //Build domain names list to display in the error
Line 118: StringBuilder domainNames = new StringBuilder();
Line 119: for(StorageDomain domain : domains) {
Line 120: domainNames.append(domain.getStorageName());
Line 137: StoragePoolIsoMap map = getStoragePoolIsoMap();
Line 138:
Line 139: changeStorageDomainStatusInTransaction(map,
StorageDomainStatus.Locked);
Line 140: //connect to the new path
Line 141: boolean hasConnectStorageSucceeded = connectToStorage();
This is the whole point of the feature - to have 2 domains copied on the
background with same uuid, and then repointing from one to another.
Which alternative do you suggest? This code is here for several patchsets
already.
Line 142: VDSReturnValue returnValueUpdatedStorageDomain = null;
Line 143:
Line 144: if(!hasConnectStorageSucceeded) {
Line 145: setSucceeded(false);
Line 195: protected void executeInNewTransaction(TransactionMethod<?>
method) {
Line 196: TransactionSupport.executeInNewTransaction(method);
Line 197: }
Line 198:
Line 199: protected boolean connectToStorage() {
This is not a new code. It has been here for several patchsets. You could have
commented on it earlier. Adding new comments on existing code that has already
been reviewed many times, especially when they are matter of style and not
critical issues causes too much iterations and context switches for a patch.
It's important to keep the review efficient for us all. Please consider that
next time when performing a review.
Line 200: ConnectStorageServerVDSCommandParameters
newConnectionParametersForVdsm =
Line 201: createParametersForVdsm(getParameters().getVdsId(),
Line 202: getParameters().getStoragePoolId(),
Line 203:
getParameters().getStorageServerConnection().getstorage_type(),
Line 206: }
Line 207:
Line 208: protected VDSReturnValue updateStatsForDomain() {
Line 209: return runVdsCommand(VDSCommandType.GetStorageDomainStats,
Line 210: new
GetStorageDomainStatsVDSCommandParameters(getVds().getId(),
getStorageDomain().getId()));
I don't understand your comment. Are you suggesting to do here something
differently? Please explain.
Line 211: }
Line 212:
Line 213: protected ConnectStorageServerVDSCommandParameters
createParametersForVdsm(Guid vdsmId,
Line 214: Guid storagePoolId,
Line 243:
Line 244: @Override
Line 245: protected Map<String, Pair<String, String>> getExclusiveLocks() {
Line 246: Map<String, Pair<String, String>> locks = new HashMap<String,
Pair<String, String>>();
Line 247: domains =
getStorageDomainsByConnId(getParameters().getStorageServerConnection().getid());
1. This is already extracted to method named getStorageDomainsByConnId which is
reused.
2. The parameters here are of a connection type. No domain is passed to them
because it's not the same entity (unfortunate , but this is the current system
design). And getExclusiveLocks is called before canDoAction and cannot return
an error to user - also unfortunate, but current design. Thus, this is a
compromise - try to lock if there is what to lock, and if not - canDoAction is
returning a proper error to user if something's wrong with configuration.
Line 248: if(!domains.isEmpty() && domains.size() == 1) {
Line 249: setStorageDomain(domains.get(0));
Line 250: locks.put(getStorageDomain().getId().toString(),
LockMessagesMatchUtil.STORAGE);
Line 251: }
--
To view, visit http://gerrit.ovirt.org/12372
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaff5344ff191d6bdf53cc706c7bb796167a56b3
Gerrit-PatchSet: 11
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alissa Bonas <abo...@redhat.com>
Gerrit-Reviewer: Alissa Bonas <abo...@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: Itamar Heim <ih...@redhat.com>
Gerrit-Reviewer: Liron Ar <lara...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
