-
Notifications
You must be signed in to change notification settings - Fork 41
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
Fix register search attributes when starting server #653
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM, but not approving because we cannot merge an untagged server version.
go.mod
Outdated
go.temporal.io/sdk v1.27.0 | ||
go.temporal.io/server v1.24.2 | ||
google.golang.org/grpc v1.65.0 | ||
go.temporal.io/server v1.26.0-120.0.20240904002151-40431ec95546 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We won't be able to merge this until it is in a tagged version. If it only lands in a non-stable tagged version (i.e. 1.26 series and not one of the 1.25 patches), we will need to change the branch this is targeting to next
(once that branch is created).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The branch exists (it's called next-server
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 So once this is in a tagged non-GA version, this can be updated and the target if this PR can be changed to target that branch (or we can close and recreate the PR if preferred).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the base branch to next-server
.
94db02c
to
92b8f1b
Compare
LGTM, but not marking this approved until on tagged server. There seem to be other tests the latest server is breaking. |
92b8f1b
to
171be43
Compare
98b686c
to
7d57555
Compare
7d57555
to
0145783
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM. We have now resolved to not make future server tags depending on SDK/API commit hashes. So when next-server
makes it into main
we should of course have stable tags for everything.
81f11b8
to
00ea0c7
Compare
What was changed
Fix register search attributes when starting server
Why?
Currently, search attributes are registered after the server is ready, which can cause race condition with other processes waiting for the server to become ready too. This PR fixes this by registering the search attributes together with the namespaces.
Address temporalio/temporal#6195
Checklist
Closes
How was this tested: