Change in vdsm[master]: Refactor storage connection subsystem

abaron at redhat.com abaron at redhat.com
Sun Jan 15 21:11:54 UTC 2012


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 at anotherthing (ex pos of @)
gluster:something at anotherthing?blabla (ex pos of ?)
gluster:something at 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 at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>


More information about the vdsm-patches mailing list