feat(s3/lifecycle): drop Worker Count knob from admin config form

The "Worker Count" admin field controlled in-process pipeline goroutines
across the 16-shard space — per-worker tuning, not a cluster-wide scope
concern. Operators looking at the form alongside Cluster Delete Rate
reasonably misread it as the number of workers in the cluster.

Drop the form field and DefaultValues entry. cfg.Workers is now hardcoded
to shardPipelineGoroutines (=1) inside ParseConfig; the rest of the
plumbing through dailyrun.Config.Workers stays so a future need can
re-introduce it as a worker-local knob (or just bump the constant).

handler_test.go pins that "workers" must NOT appear in the form so the
removal doesn't silently regress.
This commit is contained in:
Chris Lu
2026-05-11 19:15:13 -07:00
parent b85af3483e
commit 0b3d50444e
4 changed files with 19 additions and 43 deletions

View File

@@ -9,7 +9,12 @@ import (
const (
jobType = "s3_lifecycle"
defaultWorkers = 1
// shardPipelineGoroutines is the in-process fan-out across the
// 16-shard space. Kept as a hardcoded internal default — formerly
// an admin form field, removed because it's a per-worker tuning
// knob, not a cluster-coordination concern.
shardPipelineGoroutines = 1
defaultDispatchTickMinutes = int64(1)
defaultCheckpointTickSeconds = int64(30)
defaultRefreshIntervalMinutes = int64(5)
@@ -45,7 +50,7 @@ type Config struct {
// admin+worker config values. Missing fields fall back to defaults.
func ParseConfig(adminValues, workerValues map[string]*plugin_pb.ConfigValue) Config {
cfg := Config{
Workers: int(readInt64(adminValues, "workers", defaultWorkers)),
Workers: shardPipelineGoroutines,
DispatchTick: time.Duration(readInt64(workerValues, "dispatch_tick_minutes", defaultDispatchTickMinutes)) * time.Minute,
CheckpointTick: time.Duration(readInt64(workerValues, "checkpoint_tick_seconds", defaultCheckpointTickSeconds)) * time.Second,
RefreshInterval: time.Duration(readInt64(workerValues, "refresh_interval_minutes", defaultRefreshIntervalMinutes)) * time.Minute,
@@ -53,9 +58,6 @@ func ParseConfig(adminValues, workerValues map[string]*plugin_pb.ConfigValue) Co
MaxRuntime: time.Duration(readInt64(workerValues, "max_runtime_minutes", defaultMaxRuntimeMinutes)) * time.Minute,
Algorithm: readString(adminValues, "algorithm", defaultAlgorithm),
}
if cfg.Workers <= 0 {
cfg.Workers = defaultWorkers
}
if cfg.DispatchTick <= 0 {
cfg.DispatchTick = time.Duration(defaultDispatchTickMinutes) * time.Minute
}

View File

@@ -9,8 +9,8 @@ import (
func TestParseConfigDefaults(t *testing.T) {
cfg := ParseConfig(nil, nil)
if cfg.Workers != defaultWorkers {
t.Errorf("Workers default=%d, want %d", cfg.Workers, defaultWorkers)
if cfg.Workers != shardPipelineGoroutines {
t.Errorf("Workers default=%d, want %d", cfg.Workers, shardPipelineGoroutines)
}
if cfg.DispatchTick != 1*time.Minute {
t.Errorf("DispatchTick default=%v, want 1m", cfg.DispatchTick)
@@ -58,9 +58,6 @@ func TestParseConfig_AlgorithmUnknownValueFallsBackToDefault(t *testing.T) {
}
func TestParseConfigOverrides(t *testing.T) {
admin := map[string]*plugin_pb.ConfigValue{
"workers": {Kind: &plugin_pb.ConfigValue_Int64Value{Int64Value: 4}},
}
worker := map[string]*plugin_pb.ConfigValue{
"dispatch_tick_minutes": {Kind: &plugin_pb.ConfigValue_Int64Value{Int64Value: 2}},
"checkpoint_tick_seconds": {Kind: &plugin_pb.ConfigValue_Int64Value{Int64Value: 15}},
@@ -68,10 +65,7 @@ func TestParseConfigOverrides(t *testing.T) {
"bootstrap_interval_minutes": {Kind: &plugin_pb.ConfigValue_Int64Value{Int64Value: 120}},
"max_runtime_minutes": {Kind: &plugin_pb.ConfigValue_Int64Value{Int64Value: 120}},
}
cfg := ParseConfig(admin, worker)
if cfg.Workers != 4 {
t.Errorf("Workers=%d, want 4", cfg.Workers)
}
cfg := ParseConfig(nil, worker)
if cfg.DispatchTick != 2*time.Minute {
t.Errorf("DispatchTick=%v, want 2m", cfg.DispatchTick)
}

View File

@@ -74,17 +74,8 @@ func (h *Handler) Descriptor() *plugin_pb.JobTypeDescriptor {
{
SectionId: "scope",
Title: "Scope",
Description: "How many pipeline goroutines split the 16-shard space.",
Description: "Cluster-wide algorithm choice and delete-throughput cap.",
Fields: []*plugin_pb.ConfigField{
{
Name: "workers",
Label: "Worker Count",
Description: "Number of pipeline goroutines per executing worker. Each owns a contiguous slice of [0, 16) shards. Default 1 = one goroutine handles all 16 shards.",
FieldType: plugin_pb.ConfigFieldType_CONFIG_FIELD_TYPE_INT64,
Widget: plugin_pb.ConfigWidget_CONFIG_WIDGET_NUMBER,
MinValue: &plugin_pb.ConfigValue{Kind: &plugin_pb.ConfigValue_Int64Value{Int64Value: 1}},
MaxValue: &plugin_pb.ConfigValue{Kind: &plugin_pb.ConfigValue_Int64Value{Int64Value: 16}},
},
{
Name: "algorithm",
Label: "Algorithm",
@@ -116,7 +107,6 @@ func (h *Handler) Descriptor() *plugin_pb.JobTypeDescriptor {
},
},
DefaultValues: map[string]*plugin_pb.ConfigValue{
"workers": {Kind: &plugin_pb.ConfigValue_Int64Value{Int64Value: defaultWorkers}},
"algorithm": {Kind: &plugin_pb.ConfigValue_StringValue{StringValue: defaultAlgorithm}},
ClusterDeletesPerSecondAdminKey: {Kind: &plugin_pb.ConfigValue_Int64Value{Int64Value: 0}},
ClusterDeletesBurstAdminKey: {Kind: &plugin_pb.ConfigValue_Int64Value{Int64Value: 0}},

View File

@@ -284,31 +284,21 @@ func TestDescriptor_BasicShape(t *testing.T) {
assert.Greater(t, d.DescriptorVersion, uint32(0), "descriptor version must be positive (admins use it for compat)")
}
func TestDescriptor_AdminConfigFormHasWorkersField(t *testing.T) {
// Workers is the only admin-side knob today; if it disappears, the
// admin form would render empty and operators couldn't tune
// concurrency.
func TestDescriptor_AdminConfigFormHasNoWorkersField(t *testing.T) {
// "workers" used to be an admin form field controlling in-process
// pipeline goroutines. It's per-worker tuning, not a cluster-wide
// scope concern, so it was removed from the form. ParseConfig hard-
// codes cfg.Workers from shardPipelineGoroutines instead.
h := NewHandler(nil)
d := h.Descriptor()
require.NotNil(t, d.AdminConfigForm)
assert.Equal(t, "s3-lifecycle-admin", d.AdminConfigForm.FormId)
// Walk every section's fields and find "workers".
var found bool
for _, sec := range d.AdminConfigForm.Sections {
for _, f := range sec.Fields {
if f.Name == "workers" {
found = true
assert.Equal(t, plugin_pb.ConfigFieldType_CONFIG_FIELD_TYPE_INT64, f.FieldType)
}
assert.NotEqual(t, "workers", f.Name, "admin form must NOT expose the in-memory pipeline-goroutine knob")
}
}
assert.True(t, found, "admin form must expose 'workers' field")
// Default value matches the constant used by ParseConfig.
dv, ok := d.AdminConfigForm.DefaultValues["workers"]
require.True(t, ok, "workers must have a default in AdminConfigForm")
assert.Equal(t, int64(defaultWorkers), dv.GetInt64Value())
_, hasDefault := d.AdminConfigForm.DefaultValues["workers"]
assert.False(t, hasDefault, "DefaultValues must NOT include 'workers' (form field removed)")
}
func TestDescriptor_WorkerConfigFormCadenceDefaultsMatchParseConfig(t *testing.T) {