Provide a shard ownership-based rate limit quota scaler from the history/shard package #4704
Conversation
MichaelSnowden
force-pushed
the
snowden/shard-counter
branch
from
638c9b5
to
fb9fe29
Compare
| var Module = fx.Options( | ||
| fx.Provide(ControllerProvider), | ||
| fx.Provide(ContextFactoryProvider), | ||
| fx.Provide(fx.Annotate( |
There was a problem hiding this comment.
Two reasons for removing this:
- fx.Provide is variadic, so you don't need multiple statements
- fx.Options usage is discouraged; the docs read, "Use this pattern sparingly, since it limits the user's ability to customize their application." On its own, it's fine, but it allows you to embed other modules, which induces the problem that quote is talking about because you're choosing the inputs to the module for the user instead of letting them do so.
There was a problem hiding this comment.
Does fx.Provide work with Decorate or Replace? I remember using fx.Module can limit the scope of those functions, which in most cases are ideal. I kinda think we should change all var Module = fx.Options(...) in our codebase to a real fx.Module.
There was a problem hiding this comment.
Decorate and Replace can be used by clients of this module just as easily as they could if this were an fx.Module. I don't think there's a point in libraries defining their own fx.Module objects because, if a client wants to limit the scope of a graph augmentor (decorate or replace), their code will basically look the exact same: fx.Module(name, library.SomeModule, fx.{Decorate,Replace}(...)) regardless of whether the library SomeModule is defined using fx.Provide, fx.Options or fx.Module because the clients still need to define their own module around it in order to limit their augmentor's scope--they can't put anything inside of an already-defined module.
| var Module = fx.Options( | ||
| fx.Provide(ControllerProvider), | ||
| fx.Provide(ContextFactoryProvider), | ||
| fx.Provide(fx.Annotate( |
There was a problem hiding this comment.
Does fx.Provide work with Decorate or Replace? I remember using fx.Module can limit the scope of those functions, which in most cases are ideal. I kinda think we should change all var Module = fx.Options(...) in our codebase to a real fx.Module.
MichaelSnowden
force-pushed
the
snowden/shard-counter
branch
from
fb9fe29
to
417ba5c
Compare
MichaelSnowden
force-pushed
the
snowden/shard-counter
branch
from
417ba5c
to
6ed482b
Compare
| } | ||
| // shardCountSubscription is a subscription to shard count updates. | ||
| shardCountSubscription struct { | ||
| controller *ControllerImpl |
There was a problem hiding this comment.
nit: not sure if using a callback will be better here instead of referencing and mutating controller state directly, but no strong opinion.
There was a problem hiding this comment.
I tried this, but it looked too complicated
What changed?
I added code to the history/shard package which supplies the shard controller to the ownership-based rate limit quota scaler, so that it can be used to get the shard count. Basically, this PR adds the plumbing needed to propagate the shard count from the shard controller to the quota scaler, so that clients can just grab an
OwnershipBasedQuotaScalerfrom the fx graph, and then callscaler.ScaleRateBurst(rb)without thinking about any other dependencies.Why?
So that we can have rate limit quotas in the history service that scale with the approximate workload of the service.
How did you test it?
I added tests which verify that shard counts are propagated to subscribers whenever acquire shards completes. I also verified that this does not block if the subscriber is not listening.
Potential risks
The changes to ControllerImpl here come with a risk because the shard controller is such an essential part of the history service.
Is hotfix candidate?
No.