zhixinwen commented on code in PR #3075:
URL: https://github.com/apache/kvrocks/pull/3075#discussion_r2244322153
##########
src/cluster/replication.cc:
##########
@@ -654,11 +678,18 @@ ReplicationThread::CBState
ReplicationThread::incrementBatchLoopCB(bufferevent *
// master would send the ping heartbeat packet to check whether the
slave was alive or not,
// don't write ping to db here.
if (data_written) {
- sendReplConfAck(bev);
+ sendReplConfAck(bev, force_ack);
}
return CBState::AGAIN;
}
+ if (bulk_string == "_getack") {
+ // master would send the _getack command to the master to get
acknowledgment
+ // don't write _getack to db here.
+ force_ack = true;
Review Comment:
I don't think so, this would lower replication throughput. Even though it
tries to ack ASAP, but lowering throughput can actually make the end to end
latency longer.
For example,
T1: SET + WAIT is called
T2: WAL entry and _getack sent to replication
T3: Replication ack
Although the current impl increases the time between T2 & T3, the time diff
should be small. But if the throughput is lower, we can see a large gap between
T1 and T2.
I am actually seeing 500ms in T1 and T2 in my test env for some cases, I
want to follow up with https://github.com/zhixinwen/kvrocks/pull/2 and even
more aggressive batching to increase throughput and make the behavior
configurable. Users who heavily use WAIT would need to tune the settings for
their workload.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
