Support custom yarpc peer chooser for p2p connections

[taylanisikdemir]

Problem
Cadence services use yarpc's "direct chooser" implementation for p2p connections over grpc. These p2p calls happen

• from frontend to history & matching
• from history to matching
• from matching to history

Existing implementation of "direct chooser" doesn't retain connections. Closing connections immediately after each request is not ideal because same hosts will most likely keep connecting again and again.
The fix is to replace the direct chooser with a custom implementation that retains connections and closes them only when the target host disappears from peer member list.

What changed
This PR contains the prep work to be able to switch the yarpc chooser implementation with a custom one that retains connections. It will be toggleable via a dynamic config.

Changes to propagate membership updates to new chooser implementation:

• Moved RPCFactory interface from common to rpc package (where it belongs) to avoid circular dependency.
• RPC factory is now responsible from propagating membership updates to Outbounds object.
• RPC factory is a daemon now and started/stopped by resourceImpl.
• Outbounds is a new object that OutboundsBuilders return instead of yarpc.Outbounds. Outbounds encapsulates yarpc.Outbounds + UpdatePeers() func.
• Only direct outbound builder (used for p2p) registers an UpdatePeers callback to underlying Chooser.
• PeerChooserFactory now returns an internal interface PeerChooser which encapsulates yarpc's peer.Chooser + UpdatePeers() func

Added new custom chooser implementation which gets membership updates but currently it behaves as existing "direct chooser" and doesn't use membership update information. Actual implementation of the connection retaining chooser will be added in a follow up PR.

How did you test it?

• Added new unit tests
• Manually deployed to a dev environment and ran basic benchmarks to validate p2p connections still work
• Existing integration tests are passing

[codecov]

Codecov Report

Attention: Patch coverage is 80.23952% with 33 lines in your changes missing coverage. Please review.

Project coverage is 74.92%. Comparing base (100562f) to head (cc96652).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
common/rpc/direct_peer_chooser.go	70.00%	11 Missing and 4 partials ⚠️
common/rpc/factory.go	84.09%	4 Missing and 3 partials ⚠️
common/rpc/outbounds.go	88.09%	3 Missing and 2 partials ⚠️
common/resource/resourceImpl.go	0.00%	4 Missing ⚠️
common/membership/hashring.go	0.00%	1 Missing ⚠️
common/rpc/peer_chooser.go	90.90%	1 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
common/membership/resolver.go	80.72% <ø> (ø)
common/rpc/params.go	80.53% <100.00%> (+2.31%)	⬆️
common/membership/hashring.go	91.05% <0.00%> (ø)
common/rpc/peer_chooser.go	84.21% <90.90%> (+2.39%)	⬆️
common/resource/resourceImpl.go	2.86% <0.00%> (-0.02%)	⬇️
common/rpc/outbounds.go	86.17% <88.09%> (-2.18%)	⬇️
common/rpc/factory.go	69.72% <84.09%> (+65.61%)	⬆️
common/rpc/direct_peer_chooser.go	70.00% <70.00%> (ø)

... and 7 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

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

[davidporter-id-au]

LGTM.

Some questions about choices: Lazy vs upfront connection setup?

[Groxx]

Some questions about choices: Lazy vs upfront connection setup?

For now I'll vote lazy (unless upfront is easier somehow) since it's what we currently do and it's apparently fast enough. Plus, lazy is needed to some degree since hosts can enter and exit, so I would generally expect "only lazy" to be simpler and more obviously-correct (because it's always used, rather than only in edge cases / long-running deployments).

I have not read the changes yet tho, so that's just going off initial feels.

[Groxx]

And after reading: yeah I'll vote for lazy connections only / primarily. And probably idle-shutdown too.

You'll need lazy to be able to dynamically turn this on and off, and have it actually be off and not just unused.
And with an idle-connection-shutdown you don't have to do anything when going from on -> off, just let the connections close on their own.¹

Overall makes sense though. Not too bad for the part leading up to "and then draw the rest of the horse" :)

Footnotes

1. assuming yarpc doesn't mind having more than one connection to a host. I would hope that's fine though? ² ↩

2. oh neat, we have footnotes now ↩

[taylanisikdemir]

Some questions about choices: Lazy vs upfront connection setup?

For now I'll vote lazy (unless upfront is easier somehow) since it's what we currently do and it's apparently fast enough. Plus, lazy is needed to some degree since hosts can enter and exit, so I would generally expect "only lazy" to be simpler and more obviously-correct (because it's always used, rather than only in edge cases / long-running deployments).

I have not read the changes yet tho, so that's just going off initial feels.

Lazy approach is more efficient way to go. It's how it's done currently. No reason to switch to upfront conn setup with this change. I don't think it makes implementation any simpler.