Juan Hernandez has posted comments on this change.
Change subject: Run upgradeStoragePool on cluster compatibility change
......................................................................
Patch Set 9: (9 inline comments)
See my comments inside. Most of them are not very relevant.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStoragePoolCommand.java
Line 62: updateStoragePoolFormatType();
Does this go inside the if our outside the if? If it goes inside please indent
it.
To make this code clearer I would suggest to create an additional
"updateStoragePoolDescription" method, with the logic that goes inside the if.
That way you can have something like this:
if (...) {
updateStoragePoolDescritpion();
updateStoragePoolFormatType();
}
I think it is easier to understand.
Line 96: private void updateStoragePoolFormatType() {
Consider using "final" for local variables.
Line 118: Version v3FormatChangeBar = new Version("3.1");
Why not use "Version.v3_0" and "Version.v3_1" instead of creating new instances?
(and what does "ChangeBar" mean?)
Line 150: log.info(domains);
These look like debug messages, not info.
If you stick to info (may make sense, as this doesn't happens that often) I
would suggest something easier to read:
log.info("Updating storage domain \"" + domain.getId() + "\" format to \"" +
targetFormat + "\".");
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/UpgradeStoragePoolVDSCommandParameters.java
Line 21: return String.format("spUUID = %s, poolVersion = %s",
Can you put here "storagePoolId" instead of "spUUID".
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/VDSCommandType.java
Line 128: UpgradeStoragePool("org.ovirt.engine.core.vdsbroker.irsbroker"),
You may want to locate this new command type next to the other *StoragePool
commands. Makes it easier to find.
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/UpgradeStoragePoolVDSCommand.java
Line 14: P params = getParameters();
Consider adding the "final" modifier to local variables:
final P params = ...
final String spUUIDstr = ...
It won't make any difference in this case, but it can help catch some subtle
errors in general.
Line 15: String spUUIDstr = params.getStoragePoolId().toString();
spUUIDstr is not a very friendly user name. You may want to name this
"storagePoolId", makes it easier to read.
Line 22: targetVersion);
I think that in log messages like this there is no need to put the name of the
class or the name of the method, as that can be extracted by the logging
mechanism if needed. Put just the relevant information, for example:
log.info("Upgrading storage pool \"" + spUUIDstr + "\"" to version \"" +
targetVersion + "\".");
--
To view, visit http://gerrit.ovirt.org/2441
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I85201624bbb2f8c41cf3b184b89a8e199ff50e99
Gerrit-PatchSet: 9
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Laszlo Hornyak <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Mike Kolesnik <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
