Skip to content

Conversation

@wen-hui-sun
Copy link
Collaborator

Note

  1. Remove the WithContext and Context functions from EntityClient
  2. Add context as the first parameter in EntityClient

Copy link
Collaborator

@allanliu allanliu left a comment

Choose a reason for hiding this comment

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

Generally LGTM sans the 2 comments. Stamping to unblock.

Copy link
Collaborator

@jemiahw jemiahw left a comment

Choose a reason for hiding this comment

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

Looks pretty good. One question/comment about query context and the timeout. Good otherwise.

@wen-hui-sun
Copy link
Collaborator Author

Nice catch. I missed that. Allan paying attention! :)
I'm not sure we even want a timeout context here though, do we? I don't think the old code set a timeout here. The server has server side timeout and I don't want the client timing out before the server, although it's not that big a deal. Still, since the old code didn't set a timeout, maybe we continue to not set timeout, but we do pass in the ctx from the Run here.

Replied in #88 (comment)

@wen-hui-sun wen-hui-sun merged commit cb04dd0 into master May 29, 2025
1 check passed
@wen-hui-sun wen-hui-sun deleted the wenhui-context/PLAT-22444 branch May 29, 2025 19:31
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.

4 participants