Ayal Baron has posted comments on this change.
Change subject: Refactor UpdateStoragePoolCommand
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(8 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStoragePoolCommand.java
Line 44: getStoragePoolDAO().updatePartial(storagePool);
what exactly does updatePartial update? the name is not very descriptive (I
care less about the how, I care more about the what)
Line 46: if (storagePool.getstatus() != StoragePoolStatus.Up) {
The semantics here are funky. If the status is not up then we don't need to
update the storage pool description? why?
Line 87:
Even though it's java there is no reason for trailing spaces
Line 92: if (!CheckStoragePoolNameLengthValid()) {
I'm assuming the naming inconsistency was not introduced in this patch? (one
function is "Check...Valid" and another "validate..."
Line 96: if (!valdiateComptibilityVersionChange()) {
s/valdiateComp/validateCompa/
Line 100: if (!isNotLocalfsWithDefaultCluster()) {
!isNot? double negative? really??
Line 119: return false;
You're breaking the previous functionality. Normally in the engine flows test
all validations to be able to report to the user everything that needs fixing
and not just one thing and then try again and fail and so on.
(i.e. you can run multiple addCanDoActionMessage calls and user will see all
the messages)
Line 127: // check all clusters has at least the same compatibility
s/has/have/
--
To view, visit http://gerrit.ovirt.org/2275
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0978ffd7f22c19c58c8a2c6f1f9feafabb3cdf5b
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
