On Wed, Sep 16, 2009 at 11:05 AM, Michael Hanselmann <[email protected]> wrote:
> 2009/9/16 Guido Trotter <[email protected]>:
>> --- /dev/null
>> +++ b/lib/confd/client.py
>> +from ganeti.confd import PackMagic, UnpackMagic
>
> Don't do this. Never import specific names in non-test code (and try
> to avoid it even there).
>
Ok
>> +class ConfdClient:
>> + """Send queries to confd, and get back answers.
>> +
>> […]
>> +
>> + """
>> +
>> + def __init__(self, hmac_key, mc_list):
>
> No empty line before __init__ after the class docstring.
>
Ok
>> + """Constructor for ConfdClient
>> +
>> + �...@type hmac_key: string
>> + �...@param hmac_key: hmac key to talk to confd
>> + �...@type mc_list: list
>> + �...@param mc_list: list of peer nodes
>
> Why mc_? I know they're master candidates, but the client shouldn't
> care. Also you shouldn't include type names in the variable name. How
> about “peers”?
>
Ok
>> +
>> + """
>> + if not isinstance(mc_list, list):
>> + raise errors.ConfdClientError("mc_list must be a list")
>
> errors.ProgrammerError, no?
>
Ok
>> + def _PackRequest(self, request, now=None):
>> + if now is None:
>> + now = time.time()
>> + tstamp = '%s' % int(now)
>
> "%d" % now
>
Ok
>> + def HandleResponse(self, payload, ip, port):
>> + try:
>> + try:
>> + answer, salt = self._UnpackReply(payload)
>> + except errors.SignatureError, err:
>> + return
>> + except errors.ConfdMagicError, err:
>> + return
>
> except (errors.SignatureError, errors.ConfdMagicError):
>
> No need for the error variable.
>
Ok
>> +
>> + if salt in self._callbacks:
>> + callback, type, query, args = self._callbacks[salt]
>> + callback(answer, type, query, salt, ip, port, args)
>
> try:
> (callback, type, query, args) = self._callbacks[salt]
> except KeyError:
> pass
> else:
> callback(answer, type, query, salt, ip, port, args)
>
> Please also add some comments as to why you ignore unknown salts.
>
Ok
>> +
>> + finally:
>> + self._ExpireCallbacks()
>> +
>> +
>> +class ConfdClientRequest(objects.ConfdRequest):
>> + def __init__(self, **kwargs):
>> + objects.ConfdRequest.__init__(self, **kwargs)
>> + if not self.rsalt:
>> + self.rsalt = utils.NewUUID()
>
> Is this client library going to be used on non-Linux systems? NewUUID
> works only on Linux as it is now.
No, it's not. :)
Thanks, (will resend)
Guido
