Codecov Report

Merging #2099 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #2099      +/-   ##
=========================================
+ Coverage    57.2%   57.2%   +<.01%     
=========================================
  Files         117     117              
  Lines       20124   20123       -1     
=========================================
+ Hits        11511   11512       +1     
+ Misses       7285    7277       -8     
- Partials     1328    1334       +6

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2e48a3...75cc741. Read the comment docs.

I wonder if we should split this Watch RPC into its own gRPC service, separate from store. If/when we add back the other RPCs like UpdateObjects, we will want those to be proxied to the leader, because writes can only happen on the leader. But currently we can only enable/disable the raft proxy on the service level. WDYT?

I think separating Watch from Write makes sense. Watch API reading from each manager can reduce load on leader.

Do you think we should take this change while working on Write API?

Let's do it now so we don't need to make disruptive changes later. Would you mind opening a PR to change the name of what's currently the "store" service to "watch"?

I'll open a PR.

What do we gain by doing this (beside performance)?

I'm a bit afraid of API explosion.

Could this be part of control API?

Could we set the forwarding per-method rather than per-service?

Otherwise it feels like we're designing the API based on how we implemented the forwarder

Proxying watch to leader is unreliable and inefficient. The client would need to detect connection failure and reconnect to new leader. It'd result in event loss. The loss events includes "leader switch" which is an important message. It increases load on leader where each manager would connect to the leader for change notifications. The leader suffers when manager number increases (which may surprise users). We have multiple managers but almost all the loads are on leader. I think we should find ways to reduce load on leader.

I agree with @dongluochen that Watch should not be forwarded to the leader. The followers already get a stream of all the events (through Raft itself), so forcing each Watch to go through the leader puts extra load on the leader and wastes network bandwidth.

It's technically possible to change the proxying so that it can be disabled on a per-RPC basis, but I don't really like this idea. It means adding more complexity to the code generation, and making it harder to understand whether a certain RPC call will go to the leader or not. I prefer using a separate service.

Replaced by #2102.