Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vttablet throttler feature flag: -enable-lag-throttler #6815

Merged
merged 5 commits into from Oct 5, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 0 additions & 2 deletions docker/mini/vttablet-mini-up.sh
Expand Up @@ -50,8 +50,6 @@ vttablet \
-init_shard $shard \
-init_tablet_type $tablet_type \
-health_check_interval 5s \
-heartbeat_enable \
-heartbeat_interval 250ms \
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were committed earlier by mistake.

-enable_semi_sync \
-enable_replication_reporter \
-backup_storage_implementation file \
Expand Down
2 changes: 0 additions & 2 deletions examples/local/scripts/vttablet-up.sh
Expand Up @@ -47,8 +47,6 @@ vttablet \
-health_check_interval 5s \
-enable_semi_sync \
-enable_replication_reporter \
-heartbeat_enable \
-heartbeat_interval 250ms \
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were committed earlier by mistake.

-backup_storage_implementation file \
-file_backup_storage_root $VTDATAROOT/backups \
-restore_from_backup \
Expand Down
1 change: 1 addition & 0 deletions go/test/endtoend/tabletmanager/throttler/throttler_test.go
Expand Up @@ -88,6 +88,7 @@ func TestMain(m *testing.M) {
"-lock_tables_timeout", "5s",
"-watch_replication_stream",
"-enable_replication_reporter",
"-enable-lag-throttler",
"-heartbeat_enable",
"-heartbeat_interval", "250ms",
}
Expand Down
18 changes: 13 additions & 5 deletions go/vt/vttablet/tabletserver/repltracker/repltracker.go
Expand Up @@ -50,7 +50,8 @@ var (

// ReplTracker tracks replication lag.
type ReplTracker struct {
mode string
mode string
forceHeartbeat bool

mu sync.Mutex
isMaster bool
Expand All @@ -63,10 +64,11 @@ type ReplTracker struct {
// NewReplTracker creates a new ReplTracker.
func NewReplTracker(env tabletenv.Env, alias topodatapb.TabletAlias) *ReplTracker {
return &ReplTracker{
mode: env.Config().ReplicationTracker.Mode,
hw: newHeartbeatWriter(env, alias),
hr: newHeartbeatReader(env),
poller: &poller{},
mode: env.Config().ReplicationTracker.Mode,
forceHeartbeat: env.Config().EnableLagThrottler,
hw: newHeartbeatWriter(env, alias),
hr: newHeartbeatReader(env),
poller: &poller{},
}
}

Expand All @@ -88,6 +90,9 @@ func (rt *ReplTracker) MakeMaster() {
rt.hr.Close()
rt.hw.Open()
}
if rt.forceHeartbeat {
rt.hw.Open()
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when throttler runs, we need heartbeat writer to run. We do not need the reader because throttler reads directly from _vt.heartbeat table.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note that we could be calling Open and Close twice, but that is fine since they are idempotent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 that was my understanding as well.

}

// MakeNonMaster must be called if the tablet type becomes non-MASTER.
Expand All @@ -105,6 +110,9 @@ func (rt *ReplTracker) MakeNonMaster() {
// Run the status once to pre-initialize values.
rt.poller.Status()
}
if rt.forceHeartbeat {
rt.hw.Close()
}
}

// Close closes ReplTracker.
Expand Down
Expand Up @@ -82,7 +82,7 @@ func TestReplTracker(t *testing.T) {
rt.InitDBConfig(target, mysqld)
assert.Equal(t, tabletenv.Polling, rt.mode)
assert.Equal(t, mysqld, rt.poller.mysqld)
assert.True(t, rt.hw.enabled)
assert.False(t, rt.hw.enabled)
assert.False(t, rt.hr.enabled)

rt.MakeNonMaster()
Expand Down
8 changes: 8 additions & 0 deletions go/vt/vttablet/tabletserver/repltracker/writer.go
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not clear why the changes in this file are required, but I'll trust the tests :)

if config.ReplicationTracker.Mode != tabletenv.Heartbeat && !config.EnableLagThrottler {
return &heartbeatWriter{}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deepthi this change is actually reverting my previous change in the throttler PR. This if statement was originally here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(except of course now added && !config.EnableLagThrottler)

heartbeatInterval := config.ReplicationTracker.HeartbeatIntervalSeconds.Get()
return &heartbeatWriter{
env: env,
Expand Down Expand Up @@ -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
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deepthi this change is actually reverting my previous change in the throttler PR. This if statement was originally here.

if enable {
w.ticks.Start(w.writeHeartbeat)
} else {
Expand Down
5 changes: 5 additions & 0 deletions go/vt/vttablet/tabletserver/tabletenv/config.go
Expand Up @@ -142,6 +142,7 @@ func init() {

flag.BoolVar(&enableHeartbeat, "heartbeat_enable", false, "If true, vttablet records (if master) or checks (if replica) the current time of a replication heartbeat in the table _vt.heartbeat. The result is used to inform the serving state of the vttablet via healthchecks.")
flag.DurationVar(&heartbeatInterval, "heartbeat_interval", 1*time.Second, "How frequently to read and write replication heartbeat.")
flag.BoolVar(&currentConfig.EnableLagThrottler, "enable-lag-throttler", defaultConfig.EnableLagThrottler, "If true, vttablet will run a throttler service, and will implicitly enable heartbeats")

flag.BoolVar(&currentConfig.EnforceStrictTransTables, "enforce_strict_trans_tables", defaultConfig.EnforceStrictTransTables, "If true, vttablet requires MySQL to run with STRICT_TRANS_TABLES or STRICT_ALL_TABLES on. It is recommended to not turn this flag off. Otherwise MySQL may alter your supplied values before saving them to the database.")
flag.BoolVar(&enableConsolidator, "enable-consolidator", true, "This option enables the query consolidator.")
Expand Down Expand Up @@ -261,6 +262,8 @@ type TabletConfig struct {
TxThrottlerConfig string `json:"-"`
TxThrottlerHealthCheckCells []string `json:"-"`

EnableLagThrottler bool `json:"-"`

TransactionLimitConfig `json:"-"`

EnforceStrictTransTables bool `json:"-"`
Expand Down Expand Up @@ -452,6 +455,8 @@ var defaultConfig = TabletConfig{
TxThrottlerConfig: defaultTxThrottlerConfig(),
TxThrottlerHealthCheckCells: []string{},

EnableLagThrottler: false, // Feature flag; to switch to 'true' at some stage in the future

TransactionLimitConfig: defaultTransactionLimitConfig(),

EnforceStrictTransTables: true,
Expand Down
2 changes: 2 additions & 0 deletions go/vt/vttablet/tabletserver/throttle/check_result.go
Expand Up @@ -42,3 +42,5 @@ func NewErrorCheckResult(statusCode int, err error) *CheckResult {

// NoSuchMetricCheckResult is a result returns when a metric is unknown
var NoSuchMetricCheckResult = NewErrorCheckResult(http.StatusNotFound, base.ErrNoSuchMetric)

var okMetricCheckResult = NewCheckResult(http.StatusOK, 0, 0, nil)
7 changes: 6 additions & 1 deletion go/vt/vttablet/tabletserver/throttle/throttler.go
Expand Up @@ -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())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Operate() is where the throttler does all the hard work. If throttler is not enabled, we skip it altogether.

}
}

// initThrottler initializes config
Expand Down Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the throttler is disabled, Check() always responds with HTTP 200 OK

}
return throttler.check.Check(ctx, appName, "mysql", localStoreName, remoteAddr, flags)
}

Expand Down