Repository navigation
Strongly consistent Nexus endpoints lookup via dynamic config - #10208
Merged
Merged
Conversation
stephanos
force-pushed
the
stephanos/sano-flaky-endpoint-2
branch
2 times, most recently
from
May 8, 2026 23:11
a3edc65 to
6b82326
Compare
stephanos
changed the title
stephanos
force-pushed
the
stephanos/sano-flaky-endpoint-2
branch
2 times, most recently
from
May 8, 2026 23:15
fe97fdc to
619ca31
Compare
stephanos
commented
May 8, 2026
| @@ -959,6 +959,11 @@ and deployment interaction in matching and history.`, | |||
| 1*time.Second, | |||
| `RefreshNexusEndpointsMinWait is the minimum wait time between background long poll requests to update Nexus endpoints.`, | |||
| ) | |||
| RefreshNexusEndpointsOnReadForTest = NewGlobalBoolSetting( | |||
Contributor
Author
There was a problem hiding this comment.
Decided on ForTest suffix to make it very clear this is not for prod.
Contributor
There was a problem hiding this comment.
If it's not for prod, maybe it could be a test hook instead of dynamic config?
Contributor
Author
There was a problem hiding this comment.
One of the uses cases is the embedded server inside the CLI, though.
Contributor
There was a problem hiding this comment.
Yeah it may not be for prod but end users will want to set this in the dev server
stephanos
changed the title
stephanos
changed the title
stephanos
force-pushed
the
stephanos/sano-flaky-endpoint-2
branch
from
May 8, 2026 23:20
619ca31 to
3146afa
Compare
2 of 5 tasks
stephanos
force-pushed
the
stephanos/sano-flaky-endpoint-2
branch
from
May 8, 2026 23:25
3146afa to
ace3ca5
Compare
stephanos
changed the title
stephanos
commented
May 15, 2026
| @@ -163,6 +174,14 @@ func (r *EndpointRegistryImpl) GetByID(ctx context.Context, id string) (*persist | |||
| return nil, err | |||
| } | |||
|
|
|||
| if r.config.refreshOnRead() { | |||
Contributor
Author
There was a problem hiding this comment.
Even though GetByID falls back to query matching already on a cache miss, it could still provide a stale answer if the endpoint was changed/deleted.
stephanos
force-pushed
the
stephanos/sano-flaky-endpoint-2
branch
2 times, most recently
from
May 15, 2026 18:32
41d0029 to
34d8386
Compare
stephanos
force-pushed
the
stephanos/sano-flaky-endpoint-2
branch
from
May 15, 2026 18:38
34d8386 to
76e0b29
Compare
stephanos
commented
May 15, 2026
| } | ||
| return true | ||
| }, 10*time.Second, 100*time.Millisecond, "endpoint should become visible") | ||
| } |
bergundy
approved these changes
May 15, 2026
| @@ -163,6 +174,14 @@ func (r *EndpointRegistryImpl) GetByID(ctx context.Context, id string) (*persist | |||
| return nil, err | |||
| } | |||
|
|
|||
| if r.config.refreshOnRead() { | |||
chrsmith
approved these changes
May 15, 2026
chrsmith
left a comment
chrsmith
left a comment
Contributor
There was a problem hiding this comment.
Some optional nitpicks. LGTM.
| false, | ||
| `RefreshNexusEndpointsOnRead forces the Nexus endpoint registry to refresh from matching service on read. | ||
| This effectively bypasses the cache so that endpoint writes are visible to readers immediately, instead of after the | ||
| next background long-poll refresh. This should not be turned on in production.`, |
Contributor
There was a problem hiding this comment.
Nit: Someone might read this and think "why not?", so maybe add a pithy explanation? e.g. "This should not be turned on in production, as it would introduce scalability/reliability problems." What do you think?
| // This is useful for test and single-node deployments that need endpoint writes | ||
| // to be visible to GetByName immediately, without waiting for background long poll. | ||
| if err := r.loadEndpoints(ctx); err != nil { | ||
| return nil, err |
Contributor
There was a problem hiding this comment.
Nit: It would be really confusing to see an error related to loading endpoints when calling GetByName. Perhaps we should use fmt.Errorf("refreshing endpoints: %w", err) here and below?
stephanos
added a commit
to stephanos/cli
that referenced
this pull request
May 15, 2026
Matches the upstream rename in temporalio/temporal#10208 to better mirror the existing forceSearchAttributesCacheRefreshOnRead naming. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 15, 2026
stephanos
added a commit
to temporalio/cli
that referenced
this pull request
May 15, 2026
<!--- Note to EXTERNAL Contributors --> <!-- Thanks for opening a PR! If it is a significant code change, please **make sure there is an open issue** for this. We work best with you when we have accepted the idea first before you code. --> <!--- For ALL Contributors 👇 --> ## What was changed <!-- Describe what has changed in this PR --> Mirrors the existing `forceSearchAttributesCacheRefreshOnRead` default: makes Nexus endpoint writes immediately visible to readers instead of after the next background long-poll refresh. It was added here temporalio/temporal#10208 ## Why Better development experience.
stephanos
added a commit
that referenced
this pull request
May 20, 2026
## What changed? Removed retries in test when Nexus endpoint wasn't found. Follow-up to #10208 ## Why? Nexus lookups are now strongly consistent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed?
Instead of using the cache for Nexus endpoint name lookups, always look it up via RPC when
RefreshNexusEndpointsOnReadis set.See #10204 for an alternative approach.
Why?
During testing - such as functional server tests or SDK tests against the CLI - the Nexus endpoint is not always immediately available after creation. This creates friction as it requires extra retries and causes flakiness if not guarded against. This change eliminates that need.
How did you test it?