Ayal Baron has posted comments on this change.
Change subject: Refactor storage connection subsystem
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(27 inline comments)
....................................................
Commit Message
Line 9: The current one is an abysmal mess. This is a tramsitional patch
s/The current one/Currently connection management/
s/tramsitional/transitional/
Line 10: retrofitting the old API to some of the concept we intend to implement.
s/concept/concepts/
Line 19: apropriate place for that check.
s/apropriate/appropriate/
....................................................
File vdsm/storage/connectionMonitor.py
Line 38: # This are properties used to define how to parse the uri for this type of
These properties are used...
Line 39: # connection. Declaring all of them is mendatory.
s/mendatory/mandatory/
Line 56: Do not vlidate the scheme of the URI as it is declared by the monitor
object.
s/vlidate/validate/
Line 66: """Disconnect if connected. If already disconnected fail
silantly"""
s/silantly/silently/
Line 79: """Implement this! It will be used to detect multiple
connection to the
s/connection/connections/
Line 99: def parseParam(param):
what about a comment stating what param looks like?
it is unclear when reviewing the function and parseParam says very little.
given input: "a='a'&b='b'&anotherparam"
Iiuc above would yield:
((a,"a"),(b,"b"),("anotherparam"))
?
Line 130: j = r
Here we're missing several examples and expected output, something of the sort:
nfs://abcd (ex. 6?)
gluster:something@anotherthing (ex pos of @)
gluster:something@anotherthing?blabla (ex pos of ?)
gluster:something@anotherthing?blabla;blublu (ex pos of ;)
Line 137: nonHierarchical = True
double negative is annoying, why did they name it this way?
'hierarchical = False' is simpler
Line 142: localPathBase = "/tmp"
why is this hard coded?
Line 190: if self._mount.isMounted():
why not start with this check?
Line 220: DEFAULT_OPTIONS = ["soft", "nosharecache",
"vers=3"]
I thought we got rid of 'vers=3' to allow for autonegotiation by default...
Line 245: pass
what is the point of the above section? Placeholder for something that still needs to be
fixed? if so, why don't you have a FIXME?
also, you throw and catch and don't do anything with it (i.e. you ignore) is that
intentional?
Also, the comment makes no sense, I'm guessing what you meant was:
hostname:port/path ???
Line 247: if uri.username:
I'm assuming this is needed for better vers=4 support?
Line 249: if uri.password:
can password be passed without username (similar to iscsi's shared secret)
Line 250: raise ValueError("Custom login is not supported")
Again, FIXMEs?
Also, messages are not very descriptive. Consider writing something like:
mounting NFS with custom login is not supported.
Line 252: for key, val in params:
what's wrong with:
if key in ("timeout", "retrans", "version"):
kwargs[key] = int(val)
???
Line 283: usesFragment = False
add space line
Line 293: username =""
space before ""
Line 314: kwargs["target"] = uri.path[1:] # ignore the / at path start
Is there no possibility of passing path without leading '/'?
Line 325: elif key == "initiator_name":
why is the key name different than what we're passing? (and don't say backward
compatibility as uri support is not bc)
Line 437: raise ValueError("Custom port is not supported")
bad copy paste?
Line 441: if uri.username:
I've stopped reviewing here, will try to continue at a later point in time.
....................................................
File vdsm/storage/Makefile.am
Line 28: connectionMonitor.py \
what kind of connections does this monitor? I'm not sure the name is expressive
enough
Line 59: sync.py \
what sync does is wrap calls and execute async?
Unless I'm missing something this name is confusing.
asyncDispatcher or something?
--
To view, visit
http://gerrit.ovirt.org/1038
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifc5c160aba9b4cc88225dde0c44f9b766c6a4c0d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>