Improve VTOrc config handling to support dynamic variables #17218
Conversation
Signed-off-by: Manan Gupta <manan@planetscale.com>
…nce poll seconds Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
…o viper Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
GuptaManan100
requested review from
deepthi,
shlomi-noach,
timvaillancourt,
ajm188 and
mattlord
as code owners
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
vitess-bot
bot
added
NeedsBackportReason
GuptaManan100
added
Type: Feature
Component: VTorc
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17218 +/- ##
==========================================
+ Coverage 67.37% 67.41% +0.03%
==========================================
Files 1573 1573
Lines 253113 253087 -26
==========================================
+ Hits 170538 170608 +70
+ Misses 82575 82479 -96 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Signed-off-by: Manan Gupta <manan@planetscale.com>
| viperutil.Options[time.Duration]{ | ||
| FlagName: "instance-poll-time", | ||
| Default: 5 * time.Second, | ||
| Dynamic: true, |
There was a problem hiding this comment.
This makrs the field dynamic
| } | ||
|
|
||
| // acceptSighupSignal registers for SIGHUP signal from the OS to reload the configuration files. | ||
| func acceptSighupSignal() { |
There was a problem hiding this comment.
@GuptaManan100 double-checking: the 3rd party library handles SIGHUP reload?
There was a problem hiding this comment.
SIGHUP is not even required anymore. Viper has a watcher on the file, and if it changes, it automatically reloads the configurations.
There was a problem hiding this comment.
@GuptaManan100 looks good to me, added one question re: SIGHUP reload that I assume still functions
| status, resp, err := utils.MakeAPICall(t, vtorc, "/api/config") | ||
| require.NoError(t, err) | ||
| assert.Equal(t, 200, status) | ||
| assert.Contains(t, resp, `"snapshottopologyinterval": 0`) |
There was a problem hiding this comment.
Isn't the response JSON? I'm wondering why the case of the config key is not being preserved. I'd expect it to be SnapshotTopologyInterval
There was a problem hiding this comment.
Yeah, but for some reason getting all the settings from the config, it returns everything in small case. Must be some internal implementation sideeffect of viper.
| ersEnabled = true | ||
| convertTabletsWithErrantGTIDs = false |
There was a problem hiding this comment.
Why did you decide not to make these two dynamic / viper-managed? I understand why you cannot make sqliteDataFile dynamic, that makes perfect sense, however, that can also be viperized and provided via the config file.
There was a problem hiding this comment.
I had originally just changed all the configurations that could be specified in the config file to be viper enabled. I figured what else we make viper configurable would be a discussion in itself. But I can make these two to be viper enabled too.
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
…able Signed-off-by: Manan Gupta <manan@planetscale.com>
|
I added a e2e test to check that the configs are indeed reloading dynamically. I, however, only added it for one of the configs. Would you prefer if I added one for each of the reloadable configurations? |
Signed-off-by: Manan Gupta <manan@planetscale.com>
we can add all of the configs into the same test. |
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Description
This PR changes how config files are handled in VTOrc, moving them to use viper instead of the custom solution previously employed. This change allows the following parameters to be dynamically loaded -
InstancePollTimePreventCrossCellFailoverSnapshotTopologyIntervalReasonableReplicationLagAuditToBackendAuditToSyslogAuditPurgeDurationWaitReplicasTimeoutTolerableReplicationLagTopoInformationRefreshDurationRecoveryPollDurationRelated Issue(s)
Checklist
Deployment Notes