Refactor replication code #1507
Conversation
royjacobson
force-pushed
the
refactor_replica
branch
from
d9324a2 to
9555386
Compare
| /* These may help but require additional field testing to learn. | ||
| int yes = 1; | ||
| CHECK_EQ(0, setsockopt(sock_->native_handle(), IPPROTO_TCP, TCP_NODELAY, &yes, sizeof(yes))); | ||
| CHECK_EQ(0, setsockopt(sock_->native_handle(), SOL_SOCKET, SO_KEEPALIVE, &yes, sizeof(yes))); | ||
|
|
||
| int intv = 15; | ||
| CHECK_EQ(0, setsockopt(sock_->native_handle(), IPPROTO_TCP, TCP_KEEPIDLE, &intv, sizeof(intv))); | ||
|
|
||
| intv /= 3; | ||
| CHECK_EQ(0, setsockopt(sock_->native_handle(), IPPROTO_TCP, TCP_KEEPINTVL, &intv, sizeof(intv))); | ||
|
|
||
| intv = 3; | ||
| CHECK_EQ(0, setsockopt(sock_->native_handle(), IPPROTO_TCP, TCP_KEEPCNT, &intv, sizeof(intv))); | ||
| */ |
There was a problem hiding this comment.
I'd put this in an issue / discussion rather than here..
There was a problem hiding this comment.
I just moved it around :) but honestly I don't think I mind it as much
royjacobson
force-pushed
the
refactor_replica
branch
from
9555386 to
f9cc49b
Compare
src/server/protocol_client.cc
Outdated
|
|
||
| ABSL_FLAG(std::string, masterauth, "", "password for authentication with master"); | ||
|
|
||
| #define RETURN_ON_BAD_RESPONSE(x) \ |
There was a problem hiding this comment.
I'd move this to a header instead of duplicating it.
I know, it's a macro and they are evil, but duplicating macros is even more evil :)
| return server_context_; | ||
| } | ||
|
|
||
| void ResetParser(bool server_mode); |
There was a problem hiding this comment.
I think it's much clearer to define an enum (instead of a bool) for such arguments. Especially for the call sites. It's also more extensible.
Also see https://abseil.io/tips/94
BTW I realize that all this function does is pass the bool to the c'tor of RedisParser. I think we should fix that c'tor as well (obviously beyond the scope of this PR).
src/server/protocol_client.h
Outdated
| // from the sock_. The output will reside in resp_args_. | ||
| // For error reporting purposes, the parsed command would be in last_resp_ if copy_msg is true. | ||
| // If io_buf is not given, a temporary buffer will be used. | ||
| io::Result<size_t> ReadRespReply(base::IoBuf* buffer = nullptr, bool copy_msg = true); |
There was a problem hiding this comment.
ditto re/ copy_msg (see below)
src/server/protocol_client.cc
Outdated
|
|
||
| result = parser_->Parse(buffer->InputBuffer(), &consumed, &resp_args_); | ||
| if (copy_msg) | ||
| last_resp_ += std::string_view((char*)buffer->InputBuffer().data(), consumed); |
There was a problem hiding this comment.
Can we use static_cast instead of c-style cast here?
There was a problem hiding this comment.
Apparently you need reinterpret_cast in this case, lol. It's fine since I cast into char* and we don't reallyy care about encodings here, but it's nicer to be as scary about this as possible I guess.
src/server/replica.cc
Outdated
| ec = response.error(); | ||
| break; |
There was a problem hiding this comment.
Any reason not to return early here, i.e. return rc;?
|
|
||
| void DflyShardReplica::StableSyncDflyAcksFb(Context* cntx) { | ||
| constexpr size_t kAckRecordMaxInterval = 1024; |
There was a problem hiding this comment.
Move this to be a global (probably anonymous namespace) var?
|
This needs more work: I'm doing some things I'm not allowed to do with buffers in the redis replication flow. |
royjacobson
force-pushed
the
refactor_replica
branch
from
f9cc49b to
e957702
Compare
* Fix the redis replication code.
royjacobson
force-pushed
the
refactor_replica
branch
from
e957702 to
b3c3460
Compare
No functional changes.
Commit #1: Split I/O logic away
Commit #2: Split shard replication into a new class.