zhixinwen commented on code in PR #3061:
URL: https://github.com/apache/kvrocks/pull/3061#discussion_r2218130194
##########
src/cluster/replication.cc:
##########
@@ -603,6 +667,7 @@ ReplicationThread::CBState
ReplicationThread::incrementBatchLoopCB(bufferevent *
util::StringToHex(batch.Data()));
return CBState::RESTART;
}
+ data_written = true;
Review Comment:
It is possible in one `incrementBatchLoopCB` call, several
`rocksdb::WriteBatch` can be constructed from existing data in bufferevent. In
my local test env, there can can anything from 3 to 22 write batches in a
single call with an average of 15 write batches.
It is unnecessary and potentially wasteful to send an ack for each of them.
Instead, we can just write all the batches to RocksDB (assuming write to
RocksDB is fast) and send ack once.
One thing I would potentially do in a following PR is to make this behavior
configurable. e.g. user may specify ack on every send / ack on every n send or
keep the current behavior. But I would want to benchmark the throughput first.
For now, I think it is a reasonable default behavior.
--
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]
