vttablet throttler feature flag: -enable-lag-throttler #6815
Conversation
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
shlomi-noach
requested review from
derekperkins,
dkhenry,
harshit-gangal,
rohit-nayak-ps and
systay
as code owners
| @@ -50,8 +50,6 @@ vttablet \ | |||
| -init_shard $shard \ | |||
| -init_tablet_type $tablet_type \ | |||
| -health_check_interval 5s \ | |||
| -heartbeat_enable \ | |||
| -heartbeat_interval 250ms \ | |||
There was a problem hiding this comment.
These were committed earlier by mistake.
| @@ -47,8 +47,6 @@ vttablet \ | |||
| -health_check_interval 5s \ | |||
| -enable_semi_sync \ | |||
| -enable_replication_reporter \ | |||
| -heartbeat_enable \ | |||
| -heartbeat_interval 250ms \ | |||
There was a problem hiding this comment.
These were committed earlier by mistake.
| @@ -88,6 +90,9 @@ func (rt *ReplTracker) MakeMaster() { | |||
| rt.hr.Close() | |||
| rt.hw.Open() | |||
| } | |||
| if rt.forceHeartbeat { | |||
| rt.hw.Open() | |||
| } | |||
There was a problem hiding this comment.
when throttler runs, we need heartbeat writer to run. We do not need the reader because throttler reads directly from _vt.heartbeat table.
There was a problem hiding this comment.
Just a note that we could be calling Open and Close twice, but that is fine since they are idempotent.
There was a problem hiding this comment.
👍 that was my understanding as well.
| @@ -186,7 +186,9 @@ func (throttler *Throttler) initThrottleTabletTypes() { | |||
| func (throttler *Throttler) InitDBConfig(keyspace, shard string) { | |||
| throttler.keyspace = keyspace | |||
| throttler.shard = shard | |||
| go throttler.Operate(context.Background()) | |||
| if throttler.env.Config().EnableLagThrottler { | |||
| go throttler.Operate(context.Background()) | |||
There was a problem hiding this comment.
Operate() is where the throttler does all the hard work. If throttler is not enabled, we skip it altogether.
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
|
Noteworthy, when the throttler is disabled, the endpoint |
| @@ -692,6 +694,9 @@ func (throttler *Throttler) AppRequestMetricResult(ctx context.Context, appName | |||
|
|
|||
| // Check is the main serving function of the throttler, and returns a check result for this cluster's lag | |||
| func (throttler *Throttler) Check(ctx context.Context, appName string, remoteAddr string, flags *CheckFlags) (checkResult *CheckResult) { | |||
| if !throttler.env.Config().EnableLagThrottler { | |||
| return okMetricCheckResult | |||
There was a problem hiding this comment.
When the throttler is disabled, Check() always responds with HTTP 200 OK
|
This branch is incorporated inside #6547 |
|
I'm ready to have this PR reviewed and merged. |
| @@ -73,6 +73,11 @@ type heartbeatWriter struct { | |||
| // newHeartbeatWriter creates a new heartbeatWriter. | |||
| func newHeartbeatWriter(env tabletenv.Env, alias topodatapb.TabletAlias) *heartbeatWriter { | |||
| config := env.Config() | |||
|
|
|||
| // config.EnableLagThrottler is a feature flag for the throttler; if throttler runs, then heartbeat must also run | |||
There was a problem hiding this comment.
It is not clear why the changes in this file are required, but I'll trust the tests :)
| @@ -182,6 +187,9 @@ func (w *heartbeatWriter) recordError(err error) { | |||
|
|
|||
| // enableWrites actives or deactives heartbeat writes | |||
| func (w *heartbeatWriter) enableWrites(enable bool) { | |||
| if w.ticks == nil { | |||
| return | |||
| } | |||
There was a problem hiding this comment.
@deepthi this change is actually reverting my previous change in the throttler PR. This if statement was originally here.
| // config.EnableLagThrottler is a feature flag for the throttler; if throttler runs, then heartbeat must also run | ||
| if config.ReplicationTracker.Mode != tabletenv.Heartbeat && !config.EnableLagThrottler { | ||
| return &heartbeatWriter{} | ||
| } |
There was a problem hiding this comment.
@deepthi this change is actually reverting my previous change in the throttler PR. This if statement was originally here.
There was a problem hiding this comment.
(except of course now added && !config.EnableLagThrottler)
followup to #6668
This PR introduces
-enable-lag-throttlerfeature flag (bool). By default-enable-lag-throttlerisfalse.When
-enable-lag-throttler=true, the vttablet throttler functionality is activated:cc @aquarapid