Change in vdsm[master]: oop: Add an option to configure oop implementation

smizrahi at redhat.com smizrahi at redhat.com
Tue Apr 29 11:49:43 UTC 2014


Saggi Mizrahi has posted comments on this change.

Change subject: oop: Add an option to configure oop implementation
......................................................................


Patch Set 6:

(5 comments)

There seems to be some bits missing

http://gerrit.ovirt.org/#/c/26576/6/lib/vdsm/config.py.in
File lib/vdsm/config.py.in:

Line 294:         ('use_volume_leases', 'false',
Line 295:             'Whether to use the volume leases or not.'),
Line 296: 
Line 297:         ('oop_impl', 'rfh',
Line 298:             'Whether to use RFH or ioprocess implementation for oop.'),
add

.. oop. Possible values are "rfh" and "ioprocess".
Line 299:     ]),
Line 300: 
Line 301:     # Section: [addresses]
Line 302:     ('addresses', [


http://gerrit.ovirt.org/#/c/26576/6/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 359: 
Line 360:         self.__validateLvmLockingType()
Line 361: 
Line 362:         oop.setDefaultImpl(config.get('irs', 'oop_impl'))
Line 363: 
> read it in outOfProcess.py, don't think we need more places to set the defa
No don't read it there read it here.

Config should only be read in hsm or clientIF
Line 364:         self.domainStateChangeCallbacks = set()
Line 365: 
Line 366:         # cleanStorageRepoitory uses tasksDir value, this must be assigned
Line 367:         # before calling it


http://gerrit.ovirt.org/#/c/26576/6/vdsm/storage/outOfProcess.py
File vdsm/storage/outOfProcess.py:

Line 28: from remoteFileHandler import RemoteFileHandlerPool
Line 29: 
Line 30: RFH = 'rfh'
Line 31: IOPROC = 'ioprocess'
Line 32: OOP_IMPL = RFH
> shouldn't it be the value from config oop_impl?
No. Configuration should not be read here.

I know we do it 2 lines after this comment but that will need to get fixed.
Line 33: 
Line 34: DEFAULT_TIMEOUT = config.getint("irs", "process_pool_timeout")
Line 35: HELPERS_PER_DOMAIN = config.getint("irs", "process_pool_max_slots_per_domain")
Line 36: 


Line 40: 
Line 41: 
Line 42: def setDefaultImpl(impl):
Line 43:     global OOP_IMPL
Line 44:     OOP_IMPL = impl
Shouldn't be in ALL_CAPS if it's not a constant
Line 45: 
Line 46: 
Line 47: def getProcessPool(clientName):
Line 48:     try:


Line 48:     try:
Line 49:         return _proc[clientName][OOP_IMPL]
Line 50:     except KeyError:
Line 51:         with _procLock:
Line 52:             _proc[clientName] = {RFH: OopWrapper(
Why is this on by default?
Line 53:                 RemoteFileHandlerPool(HELPERS_PER_DOMAIN))}
Line 54:             if OOP_IMPL == IOPROC:
Line 55:                 try:
Line 56:                     _proc[clientName][IOPROC] = _ioproc['Global']


-- 
To view, visit http://gerrit.ovirt.org/26576
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd756afd43d23631dc7ed4bac64bec9a81b358b4
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yeela Kaplan <ykaplan at redhat.com>
Gerrit-Reviewer: Barak Azulay <bazulay at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list