IGNITE-13967 Optimize and refactor of parsing response from server.#10
IGNITE-13967 Optimize and refactor of parsing response from server.#10ivandasch wants to merge 8 commits intoapache:masterfrom
Conversation
ivandasch
force-pushed
the
ignite-13967
branch
2 times, most recently
from
3cbc0cc to
0888fa8
Compare
ivandasch
force-pushed
the
ignite-13967
branch
from
5a597b6 to
b54973c
Compare
ivandasch
force-pushed
the
ignite-13967
branch
from
5e9e0c3 to
5ce8d75
Compare
isapego
left a comment
isapego
left a comment
There was a problem hiding this comment.
Overall looks good. See my minor comments and suggestions.
|
|
||
| def __init__( | ||
| self, client: 'Client', prefetch: bytes = b'', timeout: int = None, | ||
| self, client: 'Client', timeout: float = 2.0, |
There was a problem hiding this comment.
Why timeout is float? Is it a common practice for python?
There was a problem hiding this comment.
Yep, this is seconds and fraction of seconds. i.e. timeout for 150 ms is 0.15
| with self._mux: | ||
| if self._in_use: | ||
| raise ConnectionError('Connection is in use.') | ||
| self._in_use = True |
There was a problem hiding this comment.
What is the point of this flag? I don't really get it.
| self.conn = args[0] | ||
| elif len(args) == 2: | ||
| buf = args[0] | ||
| self.conn = args[1] |
There was a problem hiding this comment.
Why don't we make a conn argument to always be a first one?
There was a problem hiding this comment.
Yes, looks like a good idea
| @property | ||
| def compact_footer(self) -> bool: | ||
| return self.conn.client.compact_footer | ||
|
|
||
| @compact_footer.setter | ||
| def compact_footer(self, value: bool): | ||
| self.conn.client.compact_footer = value | ||
|
|
There was a problem hiding this comment.
It looks like we only need a conn here for binary configuration parameters. Maybe it makes sense to make a separate class for that? Like, BinaryContext, BinaryMetaProvider or something like that?
There was a problem hiding this comment.
Original author use this property as auto-discovery of compact-footer for server.
I suppose that create separate class for 1 param seems like overengineering here.
2 participants
No description provided.