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

[Feature] Lack of context.Context support #350

Open
egasimov opened this issue Sep 5, 2024 · 5 comments
Open

[Feature] Lack of context.Context support #350

egasimov opened this issue Sep 5, 2024 · 5 comments

Comments

@egasimov
Copy link

egasimov commented Sep 5, 2024

Background:
NebulaGraph's Golang client SDK (nebula-go) relies on vesoft-inc/fbthrift for Thrift communication. The ctx.Context feature in Golang provides built-in support for request cancellation, timeouts, and context propagation, which are essential for gracefully shutting down operations or user-initiated cancellations. However, it appears that the version of fbthrift used by nebula-go is outdated compared to Facebook's more recent version, where context-related functionality has been added.

Key Points:
Lack of ctx.Context Support in Nebula-Go: The Nebula-go client SDK currently does not support ctx.Context, meaning users cannot easily control timeouts, cancellations, or request interruptions during query executions. This impacts the SDK's ability to handle scenarios like user cancellations, timeout handling, or graceful shutdowns.

Outdated vesoft-inc/fbthrift Library: The vesoft-inc/fbthrift library that Nebula-go uses is out of sync with the original Facebook fbthrift. The desired ctx.Context feature was recently added in Facebook’s fbthrift, but those changes have not been merged into vesoft-inc/fbthrift.

Questions:
Synchronization of vesoft-inc/fbthrift with Facebook’s fbthrift: Is there a plan or possibility for the vesoft-inc/fbthrift library to be synchronized with the current Facebook fbthrift? Keeping the two versions in sync would allow the Nebula-go SDK to benefit from the latest improvements, including support for context headers, without manual intervention.

Cherry-picking the Desired Commit: If full synchronization is not feasible, could the commit that adds ctx.Context support in Facebook’s fbthrift be cherry-picked into vesoft-inc/fbthrift? This would be a faster way to incorporate the context-related functionality, ensuring that Nebula-go can support request cancellation and timeouts without a complete overhaul of the library.

This would be an essential feature for the SDK to handle time-sensitive operations effectively, especially in production environments requiring robust error handling and resource management.

Conclusion:
The addition of ctx.Context in Nebula-go’s SDK would greatly improve its usability by aligning it with Go’s native concurrency patterns. To achieve this, either syncing vesoft-inc/fbthrift with Facebook’s version or cherry-picking the relevant commit would enable the Golang client SDK to support these much-needed context-related features.

@egasimov egasimov changed the title [Feature] Lack of ctx.Context support [Feature] Lack of context.Context support Sep 5, 2024
@haoxins
Copy link
Collaborator

haoxins commented Sep 5, 2024

+1 for this as I will have use cases requiring request cancellation (timeout) on the recent scenarios.
I'm not sure of the background of the facebook/fbthrift fork, but it's not a long-term maintenance solution.

@egasimov
Copy link
Author

egasimov commented Sep 6, 2024

After checking the thrift protocol implementations. We have seeen that nebula-go currently utilizing the vesoft/fbthrift which was the fork facebook/fbthrift, both neither support request cancellation through context.Context.

facebook/fbthrift added support for context.Context just for transmission of request scoped values through headers.

Recently, we had a chance to look for alternative thrift implementations, and encountered with apache/thrift which it seems there is already support for request cancellation.apache/thrift - header_transport.go - currently transport mechanism utilized in the nebula-go SDK.

Waiting feedback from Nebula Developers(maybe @wey-gu please 🙏 ) to share their thoughts related to the matter.

@haoxins
Copy link
Collaborator

haoxins commented Sep 6, 2024

Recently, we had a chance to look for alternative thrift implementations

I'm not an expert on thrift, but I found another project that may promised for Go especially.

https://github.com/cloudwego/thriftgo

@Nicole00
Copy link
Contributor

Nicole00 commented Sep 9, 2024

@egasimov Thank you for the great suggestion! It is a very good and useful feature. We will confirm according to the development plan whether to include this feature and the synchronous maintenance of fbthrift in the next version.

@egasimov
Copy link
Author

egasimov commented Sep 9, 2024

You are welcome. @Nicole00
I've just prepared one more solution as PR: #351 for the above discussed feature. Feel free to review and share feedbacks. Thanks!

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

No branches or pull requests

3 participants