Skip to content
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

Use caller object as callback listener ID #2734

Merged
merged 2 commits into from
Apr 19, 2022
Merged

Conversation

yux0
Copy link
Contributor

@yux0 yux0 commented Apr 19, 2022

What changed?
Use UUID as callback listener ID

Why?
Race conditional may happen if a new shard starts before the old one close

How did you test it?
Use current unit tests

Potential risks

Is hotfix candidate?

@yux0 yux0 requested a review from a team as a code owner April 19, 2022 16:34
Copy link
Member

@dnr dnr left a comment

Choose a reason for hiding this comment

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

why not change the key to any and use the object itself as the key? (I made the same comment here: #2733 (comment))

@yux0
Copy link
Contributor Author

yux0 commented Apr 19, 2022

why not change the key to any and use the object itself as the key? (I made the same comment here: #2733 (comment))

Make sense

@meiliang86
Copy link
Contributor

You might want to update the title of the PR as you are not using UUID anymore.

@yux0 yux0 changed the title Use UUID as callback listener ID Use caller object as callback listener ID Apr 19, 2022
@@ -130,7 +130,7 @@ type (
versionToClusterName map[int64]string

clusterCallbackLock sync.RWMutex
clusterChangeCallback map[string]CallbackFn
clusterChangeCallback map[any]CallbackFn
Copy link
Contributor

Choose a reason for hiding this comment

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

comparable?

Copy link
Member

Choose a reason for hiding this comment

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

it need to be comparable. not sure any would work.

Copy link
Member

@yycptt yycptt Apr 19, 2022

Choose a reason for hiding this comment

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

I believe any here is an alias for interface{} which is a type name and comparable. comparable is a constraint which doesn't work here (can't be used as a type name).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yycptt is right. And it works here as it uses the struct pointer as key.

@yux0 yux0 merged commit 6deeed5 into temporalio:master Apr 19, 2022
@yux0 yux0 deleted the callback-id branch April 19, 2022 18:33
yycptt pushed a commit that referenced this pull request Apr 19, 2022
* Use struct as callback listener ID
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.

None yet

6 participants