Add coordinator connection reusing #6179
Conversation
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
BorysTheDev
force-pushed
the
add_coordinator_connection_reusing
branch
from
a897b5e to
e778a76
Compare
src/server/take_resp_expr.h
Outdated
| class TakeRespExpr { | ||
| public: |
There was a problem hiding this comment.
- Take is a verb, its better to be Taken or Owned
- Is it temporary? Why a new class? Why in /server?
RespExpr isn't much used besides the connection code (and tests), maybe you can modify the original one to have an owned buffer?
There was a problem hiding this comment.
I've added it here because of protocol client
There was a problem hiding this comment.
for further developement we need better command parsing approach and I think it will be more code, maybe I will create a separate directory and move this code there.
src/server/protocol_client.cc
Outdated
| return res; | ||
| } | ||
|
|
||
| io::Result<TakeRespExpr::Vec> ProtocolClient::TakeRespReply(base::IoBuf* buffer, bool copy_msg) { |
There was a problem hiding this comment.
its the same as ReadRespExpr except some small details. If this code is not temporary, please keep it as one function. Futhermore, if you extend RespExpr to be owning, you can pass it just as a parameter without any templates
BorysTheDev
force-pushed
the
add_coordinator_connection_reusing
branch
from
e778a76 to
d74ef3a
Compare
BorysTheDev
force-pushed
the
add_coordinator_connection_reusing
branch
from
d74ef3a to
b3f6299
Compare
dranikpg
left a comment
dranikpg
left a comment
There was a problem hiding this comment.
I'm not going to force my opinion on you any further but it seems like the separate type is redundant
| using Vec = std::vector<OwnedRespExpr>; | ||
| Type type; | ||
|
|
||
| std::variant<int64_t, double, Buffer, Vec> u; |
There was a problem hiding this comment.
I still don't understand why you can't extedn RespExpr with an OwnedBuffer type and then adapt TakeRespReply to conditionally use either of those
There was a problem hiding this comment.
will do in one of the next PRs
related to #6009
Added coordinator connection reusing