Skip to content

Make MuxManagerStartDelay configurable#231

Open
hehaifengcn wants to merge 3 commits into
mainfrom
haifengh/fast-start
Open

Make MuxManagerStartDelay configurable#231
hehaifengcn wants to merge 3 commits into
mainfrom
haifengh/fast-start

Conversation

@hehaifengcn
Copy link
Copy Markdown
Collaborator

@hehaifengcn hehaifengcn commented May 14, 2026

What was changed

Why?

not wait for local test.

Checklist

  1. Closes

  2. How was this tested:

  1. Any docs updates needed?

@hehaifengcn hehaifengcn requested a review from a team as a code owner May 14, 2026 23:26
@hehaifengcn hehaifengcn force-pushed the haifengh/fast-start branch from 27c3781 to 126cee7 Compare May 15, 2026 00:15
Copy link
Copy Markdown
Contributor

@liam-lowe liam-lowe left a comment

Choose a reason for hiding this comment

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

I think there is either a simpler or more optimal approach here - either feed through standard config route, or implement dynamic "readiness"

Comment thread common/global_policy.go
// GlobalPolicy is the process-wide policy singleton.
var GlobalPolicy = &Policy{
MuxManagerStartDelay: time.Minute,
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we want to introduce a global policy here - I don't want to conflate our existing configuration approach

Comment thread config/config.go
// initial connections before serving. Accepts Go duration strings
// (e.g. "30s", "1m", or "0s" to skip the wait entirely). When the
// field is omitted, the in-process default (time.Minute) is used.
MuxManagerStartDelay *Duration `yaml:"muxManagerStartDelay,omitempty"`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

top-level?

@@ -1,3 +1,4 @@
muxManagerStartDelay: "0s"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

will 0 lead to race conditions - otherwise why was it set?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

only for testing. In test code, the value is ready set to 0

@hehaifengcn
Copy link
Copy Markdown
Collaborator Author

I think there is either a simpler or more optimal approach here - either feed through standard config route, or implement dynamic "readiness"

@hehaifengcn hehaifengcn reopened this May 19, 2026
@hehaifengcn
Copy link
Copy Markdown
Collaborator Author

I think there is either a simpler or more optimal approach here - either feed through standard config route, or implement dynamic "readiness"

The main purpose of this PR is to speed up local testing by avoiding the fixed 1-minute wait, following the existing pattern used in the test code. Longer term, I’d like to remove this delay entirely — potentially via dynamic “readiness” as you suggested — so this setting is intended as a temporary workaround. Thefore, I’d prefer to avoid wiring it into the official configuration path.

hehaifengcn and others added 3 commits May 19, 2026 14:05
- common/global_policy.go: drop the now-redundant doc comments on Policy
  and UpdateMuxManagerStartDelay; the field-level docs already cover the
  semantics.
- config/config_test.go: merge TestDurationUnmarshalYAML_NestedStructAndOmitted
  into TestDurationUnmarshalYAML by always unmarshaling through a wrapper
  struct. Adds an "absent_field" row that the old top-level scalar form
  couldn't express.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@hehaifengcn hehaifengcn force-pushed the haifengh/fast-start branch from 9b6c7cd to 8e403ff Compare May 19, 2026 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants