Improve replication performance after introducing replica ack messages

[PragmaTwice]

The last question from me, I'm wondering if the replication has a noticeable slow-down due to a new blocking IO (sendString, take longer for one round in incrementBatchLoopCB since we have IO in the end?) and more network traffic?

TLDR:
Good point, you are right it would have a significant impact (40%) on replication throughput.
I will follow up with a PR to make the ack behavior configurable, so users can balance between WAIT latency and replication throughput themselves.

Full version:
I measure the replication throughput difference by comparing master and slave sequence difference every 5s when I am sending approximately 310MB/s traffic to master (80k QPS with 4kb payload). I notice the unstable branch lag increases by ~84k sequence every 5s (66MB/S) and this branch lag increases by ~220k sequence every 5s (171 MB/s). Therefore, the current unstable branch has a rough replication throughput of 240MB and this branch has throughput of roughly 140MB.

This overhead is beyond my expectation, it may take me a some time to figure out why the perf hit is so bad (btw, I dont think sendString is blocking because it just adds to the buffer?). Meanwhile, I think it makes sense to make the ack behavior configurable. For our usecase, we want to call WAIT on every write for lossless-data replication, so we want faster ack. I understand most of the users don't need this, so an ack every second or a few dozens of updates is good enough for them.

Originally posted by @zhixinwen in #3061 (comment)

[PragmaTwice]

I'm not familiar to the code of replication part, but I can try to propose a solution for this problem.

Introducing two configuration options:

replication_ack_duration (in milliseconds/seconds?)
replication_ack_seq_threshold

Algorithm:

• In IncrementBatchLoop
  • if current_seq - last_reported_seq > replication_ack_seq_threshold:
    • ACK current_seq
    • update last_reported_seq
• In the ack timer (triggered in every replication_ack_duration)
  • if last_reported_seq != current_seq:
    • ACK current_seq
    • update last_reported_seq

Current implementation is:

• replication_ack_duration = inf
• replication_ack_seq_threshold = 0

Remaining problems:

• How to tune these two values? (performance v.s. WAIT latency)
• Should we combine it with active fetching (GETACK)?
• What is the current behavior? e.g. how many ACK is sent in 1s?

cc @zhixinwen @git-hulk

[git-hulk]

Should we combine it with active fetching (GETACK)?

I think it should combine the GETACK to fetch the replica actively, or the performance of WAIT should be unacceptable for users.

In Redis, the replica sends the REPLCONF ACK every second, and the master would send the REPLCONF GETACK before entering the wait mode in the WAIT command.

[zhixinwen]

replication_ack_duration = inf
replication_ack_seq_threshold = 0

This would have an issue. If duration is inf and write stops before replication_ack_seq_threshold is met, the replica would never send ack for those writes.

Also, replication_ack_seq_threshold is not 100% the same as the current impl. In the current IncrementBatchLoop call, there can be multiple write batches processed, it would only send ack once in IncrementBatchLoop. If I understand the algorithm correctly, replication_ack_seq_threshold=0 would make one function call send multiple acks which can further hurt performance.

I think a slight change should work:

// if replication_ack_seq_buffer = 0, this value is ignored and can be unset (-1)
// otherwise this value must be set
replication_ack_interval
// -1 means disabled and rely on replication_ack_interval only
//  0 means read and process all of the data in `IncrementBatchLoop` call and send ack.
// N >= 1 means ack after N changes
replication_ack_seq_buffer 

The current unstable branch behavior is equivalent to:

replication_ack_interval: -1
replication_ack_seq_buffer: 0

By default, we can set it as:

replication_ack_interval: 15s // something close to metrics scrape interval
replication_ack_seq_buffer: -1

so it would minimize the ack overhead.

REPLCONF GETACK can be much more tricky. My initial attempt failed. I think it can be an issue when slave cannot catch up with master (so even if master send REPLCONF GETACK it would not get unblocked). I think we can implement replication_ack_interval and replication_ack_seq_buffer first to unblock the future releases and treat REPLCONF GETACK as an optimization.

What do you think?

[PragmaTwice]

If I understand the algorithm correctly, replication_ack_seq_threshold=0 would make one function call send multiple acks which can further hurt performance.

Hmmm nope. There should be at most one ACK in one call of IncrementBatchLoop.

However, I don't use a count of times of IncrementBatchLoop is called, but use the difference on sequence numbers. This is because in my mind, the data changes may not be in same size in each IncrementBatchLoop (correct me if I'm wrong). Hence, in the first call the increase of seq number can be 1, but in the second call it may be 10. It should be better if we treat them differently, so a difference between two seq numbers can describe them well. For example, if the data changes are heavy, we can have more quick ACKs so that the WAIT command can perform better.

Although I should admit that this design is tricky and hard to tune.

[zhixinwen]

Thanks for clarification. In this case, fundamentally our approaches are the same. The only difference is in your approach you are checking if ack should be sent only at the end of IncrementBatchLoop, and in my approach I may send an ack in the mid of IncrementBatchLoop call after a DB write happens. My usecase is a bit extreme, I will need to call WAIT for evey wrie calls, so calling ack on each write may make sense in my usecase. I will do some benchmark and see if it really makes sense.

Despite the difference, we can do a combination of our approaches:

// if replication_ack_seq_buffer = 1, this value can be unset (-1)
// otherwise this value must be set
replication_ack_interval

// -1 means disabled and rely on replication_ack_interval only
// N >= 1 means ack after N changes
// TBD: check the threshold at the end of `IncrementBatchLoop` or in the mid of `IncrementBatchLoop`.
replication_ack_seq_threshold

The current unstable branch behavior is equivalent to:

replication_ack_interval: -1
replication_ack_seq_buffer: 1

Default value would be:

replication_ack_interval: 15s // something close to metrics scrape interval
replication_ack_seq_buffer: -1

Once I have metrics, I will report back what choice should be made for the TBD part.

[zhixinwen]

Another approach I can think about:

1. Master knows the wait it is blocked by and the sequnce number needed to unblock a wait.
2. When master send a write to replica, it knows if the replica would unblock it after processing the write.
3. When such write is sent to the replica, the master can immediately send REPLCONF GETACK. The hope is replica would see REPLCONF GETACK immediately after it processes the write that would potentially unblock master.
4. By default, replica just ack every 1s (or something configurable)

We need to be careful to not to send multiple REPLCONF GETACK for the same WAIT. Will try this as well.