Ayal Baron has posted comments on this change.
Change subject: webadmin, engine: Use NFS v3 as default
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(6 inline comments)
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/ConnectStorageServerVDSCommand.java
Line 40: }
Line 41: return result;
Line 42: }
Line 43:
Line 44: private static void addOrDefault(Map<String, String> map, Object
what, String alt, String name) {
function signature seems a bit funky to me.
I would imagine it to be:
private static void put(Map<String, String> map, String key, Object value,
String default)
In addition, although the subject of a different patch probably, I don't
understand why we have these helper methods and not a map class with put method
overloaded to accept an additional default param.
Line 45: map.put(name, what != null ? what.toString() : alt);
Line 46: }
Line 47:
Line 48: private static void addOrEmpty(Map<String, String> map, String
what, String name) {
Line 44: private static void addOrDefault(Map<String, String> map, Object
what, String alt, String name) {
Line 45: map.put(name, what != null ? what.toString() : alt);
Line 46: }
Line 47:
Line 48: private static void addOrEmpty(Map<String, String> map, String
what, String name) {
it's bad enough to have one such function, why have 2? just change the
addOrEmpty calls to addOrDefault with "" as default to make it explicit
('Empty' is ambiguous)
Line 49: addOrDefault(map, what, "", name);
Line 50: }
Line 51:
Line 52: private static void addIfNotNullOrEmpty(Map<String, String> map,
String what, String name) {
Line 60: map.put(name, what.toString());
Line 61: }
Line 62: }
Line 63:
Line 64: public static Map<String, String> CreateStructFromConnection(final
storage_server_connections connection,
again not this patch, but wouldn't it make more sense to have a 'toHash' method
in storage_server_connections ? (and I don't understand the plural form here if
it's only 1).
Nor do I understand why the properties are allowed to have 'NULL' to begin with
and not "" wherever relevant and avoid all this mucking around with converting
NULL to ""
Line 65: final storage_pool storage_pool) {
Line 66: // for information, see _connectionDict2ConnectionInfo in
vdsm/storage/hsm.py
Line 67: Map<String, String> con = new HashMap<String, String>();
Line 68: addOrDefault(con, connection.getid(), Guid.Empty.toString(),
"id");
Line 79: // For mnt_options and vfs_type - if they are null or empty
Line 80: // we should not send a key with an empty value
Line 81: addIfNotNullOrEmpty(con, connection.getMountOptions(),
"mnt_options");
Line 82: addIfNotNullOrEmpty(con, connection.getVfsType(),
"vfs_type");
Line 83: // For protocol_version - null in db indicates auto
version negotiation
what makes V3 the default?
what happens with existing domains after upgrade, isn't the value in the db
null? (in which case it would be 'auto version negotiation' according to
comment above
Line 84: addOrDefault(con, connection.getNfsVersion(), "auto",
"protocol_version");
Line 85: addIfNotNullOrEmpty(con, connection.getNfsTimeo(),
"timeout");
Line 86: addIfNotNullOrEmpty(con, connection.getNfsRetrans(),
"retrans");
Line 87: }
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/storage/NfsStorageModel.java
Line 141: // Initialize version list.
Line 142: setVersion(new ListModel());
Line 143:
Line 144: List<EntityModel> versionItems = new ArrayList<EntityModel>();
Line 145: versionItems.add(new EntityModel(constants.nfsVersion3(),
(short) 3));
is the first element the default? if so it is not clear from the code at all
and can easily lead to mistakes.
Line 146: versionItems.add(new EntityModel(constants.nfsVersion4(),
(short) 4));
Line 147: versionItems.add(new
EntityModel(constants.nfsVersionAutoNegotiate(), null));
Line 148: getVersion().setItems(versionItems);
Line 149:
Line 143:
Line 144: List<EntityModel> versionItems = new ArrayList<EntityModel>();
Line 145: versionItems.add(new EntityModel(constants.nfsVersion3(),
(short) 3));
Line 146: versionItems.add(new EntityModel(constants.nfsVersion4(),
(short) 4));
Line 147: versionItems.add(new
EntityModel(constants.nfsVersionAutoNegotiate(), null));
why is auto stored as null and not 'auto'? the field in the db is numeric?
we will probably need to support nfs v4.1 sometime in the future and pNFS so it
shouldn't be numeric
Line 148: getVersion().setItems(versionItems);
Line 149:
Line 150: setRetransmissions(new EntityModel());
Line 151: setTimeout(new EntityModel());
--
To view, visit http://gerrit.ovirt.org/8242
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I831ecc7050a5487d5365efb94342a3c170dddc6c
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett <gpadg...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@redhat.com>
Gerrit-Reviewer: Greg Padgett <gpadg...@redhat.com>
Gerrit-Reviewer: Liron Aravot <lara...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
