Allon Mureinik has posted comments on this change.

Change subject: engine: Improve of RecoveryStoragePool
......................................................................


Patch Set 2: (3 inline comments)

....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RecoveryStoragePoolCommand.java
Line 113:                                 
getParameters().setStorageDomainId(getStorageDomainId().getValue());
Line 114:                                 try {
Line 115:                                     executeReconstruct();
Line 116:                                 } finally {
Line 117:                                     if (!reconstructOpSucceeded) {
You removed the explicit set of the StoragePoolStatus in case the reconstruct 
did not succeed.

Not necessarily saying this is wrong, but in this case, what sets it? in what 
status will it be left?
Line 118:                                         
getStoragePoolIsoMapDAO().remove(domainPoolMap.getId());
Line 119:                                     }
Line 120:                                 }
Line 121:                             } else {


....................................................
Commit Message
Line 4: Commit:     Michael Kublin <mkub...@redhat.com>
Line 5: CommitDate: 2013-02-10 13:49:29 +0200
Line 6: 
Line 7: engine: Improve of RecoveryStoragePool
Line 8: 
General: these lines are really long, please adhere to the 72 chars limits and 
split in to a a couple of lines.
Line 9: The following is done:
Line 10: 1. Moved connect storage inside code of event call
Line 11: 2. Removed useless null pointer check (copy paste code)
Line 12: 3. Removed useless update of storage pool status. No reason to update 
it


Line 10: 1. Moved connect storage inside code of event call
Line 11: 2. Removed useless null pointer check (copy paste code)
Line 12: 3. Removed useless update of storage pool status. No reason to update 
it
Line 13:    after finishing operation.
Line 14: 4. Revert of the chenges which are done in commit 
916d7df48aba8276f8d3fde884f21e19c6f77a42, the reason
s/chenges/changes/
Line 15:    is simple - commit is makes code unreadable and introducing 
regresion (send disconnect to previous master domain),
Line 16:    also I don't understand a motivation for RecoveryStoragePool to 
extend a ReconstructMasterDomainCommand and
Line 17:    in excecute create a new instance of such class - these is bad 
style of coding
Line 18: 


--
To view, visit http://gerrit.ovirt.org/11868
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0781e45b8ae76abe66fffcfad6654d27342a7c45
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Michael Kublin <mkub...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@redhat.com>
Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com>
Gerrit-Reviewer: Eli Mesika <emes...@redhat.com>
Gerrit-Reviewer: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Michael Kublin <mkub...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to