[Feature] Lack of context.Context support #350

[egasimov]

What type of PR is this?

• enhancement +1

What problem(s) does this PR solve?

Issue(s) number: #350

Description:

Add support of context.Context for operations provided by nebula-go client SDK

How do you solve it?

Using apache/thrift library for code generation part for golang SDK that supports request cancellation through context.Context

Special notes for your reviewer, ex. impact of this fix, design document, etc:

• IDL contracts are taken from nebula-repostiory
• This enhancement will cause breaking change that modifies the signature of functions(+ methods)
• Could'nt find the the following operation ExecuteWithTimeout from IDL thrift contracts

[CLAassistant]

All committers have signed the CLA.

[Nicole00]

Sorry that we cannot replace the thrift with apache thrift, we need to keep the rpc protocol the same with the server to avoid some incompatibility issues.

[Nicole00]

We will synchronize the vesoft-inc/fbthrift with facebook/fbthrift, then could you please update the pr with fbthrift? Thanks very much for your contribution~

[egasimov]

Sorry that we cannot replace the thrift with apache thrift, we need to keep the rpc protocol the same with the server to avoid some incompatibility issues.

If you don’t mind, I would like to share few thoughts regarding usage of single thrift code generators library.

1 . Hopefully, client-server interaction is done through RPC, and well defined contracts enables to develop different language SDKs via code generator tools(like facebook/fbthrift, apache/thrift).

Because thrift(RPC) is just protocol, it is hard to find one thrift-code generator that meet all the requirements for all languages.(sad reality :/)

As we already know that previously used vesoft-inc/thrift library’s generates golang client code does not contain desired functionalities, it does not have rich functionalities compared to apache/thrift.(maybe clouedwego/thriftgo contain more rich functionalities for golang code)

If you would ask my perspective regarding this approach(utilizing just vesoft-inc/thrift:fork of facebook/thrift), I would prefer to utilize different thrift implementations(apache/thrift, cloudwego/thriftgo etc)which best suits the expectations, across different programming languages.

That’s the my point of view regarding the using and sticking into the one code-generator tool.

2 . On the other side, maintaining the product(client-SDK) as well as its utilized library(vesoft-inc/fbthrift
) parallel is hard task, and I think that is not a long term solution: will bring additional effort(+maintenance(security , new features etc) cost).

Thank you for your time!

P:S If you change the mind in future, I am here to support as well as contribute.

[egasimov]

We will synchronize the vesoft-inc/fbthrift with facebook/fbthrift, then could you please update the pr with fbthrift? Thanks very much for your contribution~

TL;DR Just synchronizing the vesoft-inc/fbthrift with facebook/fbthrift is not enough, it will needed to add additional code that support context cancellation

3 . 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.

[egasimov]

Hi @Nicole00 ,
Is there any updates regarding PR + shared concerns above ?

[Nicole00]

Sorry that we cannot replace the thrift with apache thrift, we need to keep the rpc protocol the same with the server to avoid some incompatibility issues.

If you don’t mind, I would like to share few thoughts regarding usage of single thrift code generators library.

1 . Hopefully, client-server interaction is done through RPC, and well defined contracts enables to develop different language SDKs via code generator tools(like facebook/fbthrift, apache/thrift).

Because thrift(RPC) is just protocol, it is hard to find one thrift-code generator that meet all the requirements for all languages.(sad reality :/)

As we already know that previously used vesoft-inc/thrift library’s generates golang client code does not contain desired functionalities, it does not have rich functionalities compared to apache/thrift.(maybe clouedwego/thriftgo contain more rich functionalities for golang code)

If you would ask my perspective regarding this approach(utilizing just vesoft-inc/thrift:fork of facebook/thrift), I would prefer to utilize different thrift implementations(apache/thrift, cloudwego/thriftgo etc)which best suits the expectations, across different programming languages.

That’s the my point of view regarding the using and sticking into the one code-generator tool.

2 . On the other side, maintaining the product(client-SDK) as well as its utilized library(vesoft-inc/fbthrift ) parallel is hard task, and I think that is not a long term solution: will bring additional effort(+maintenance(security , new features etc) cost).

Thank you for your time!

P:S If you change the mind in future, I am here to support as well as contribute.

Thank you very much for your sharing. Indeed, keeping thrift synchronized in real time is not an easy task, and this is not part of our planned work.
The reason why there is vesoft-inc/thrift is because we found that there is a counting bug in facebook/thrift when using http2. In order to meet the needs of nebula-go to support http2, we fixed this bug in the fork repository.

[egasimov]

Sorry that we cannot replace the thrift with apache thrift, we need to keep the rpc protocol the same with the server to avoid some incompatibility issues.

If you don’t mind, I would like to share few thoughts regarding usage of single thrift code generators library.
1 . Hopefully, client-server interaction is done through RPC, and well defined contracts enables to develop different language SDKs via code generator tools(like facebook/fbthrift, apache/thrift).
Because thrift(RPC) is just protocol, it is hard to find one thrift-code generator that meet all the requirements for all languages.(sad reality :/)
As we already know that previously used vesoft-inc/thrift library’s generates golang client code does not contain desired functionalities, it does not have rich functionalities compared to apache/thrift.(maybe clouedwego/thriftgo contain more rich functionalities for golang code)
If you would ask my perspective regarding this approach(utilizing just vesoft-inc/thrift:fork of facebook/thrift), I would prefer to utilize different thrift implementations(apache/thrift, cloudwego/thriftgo etc)which best suits the expectations, across different programming languages.
That’s the my point of view regarding the using and sticking into the one code-generator tool.
2 . On the other side, maintaining the product(client-SDK) as well as its utilized library(vesoft-inc/fbthrift ) parallel is hard task, and I think that is not a long term solution: will bring additional effort(+maintenance(security , new features etc) cost).
Thank you for your time!
P:S If you change the mind in future, I am here to support as well as contribute.

Thank you very much for your sharing. Indeed, keeping thrift synchronized in real time is not an easy task, and this is not part of our planned work. The reason why there is vesoft-inc/thrift is because we found that there is a counting bug in facebook/thrift when using http2. In order to meet the needs of nebula-go to support http2, we fixed this bug in the fork repository.

Regarding bugs for http2, alternative code generator libraries(such as apache/thrift, cloudwego/thriftgo might be considered.

If you consider for using other code-generator library(rather than facebook/fbthrift), just let me know. I would be happy to contribute project. Thanks!

[Nicole00]

We are very sorry to tell you that we will not change the fbthrift for version 3.x, the changes are too big and uncertain.

[egasimov]

I understand the concern regarding the replace fbthrift 3.x due to given the significant changes it introduces. My goal with this PR was to address the lack of ctx.Context support, which is crucial for request cancellation and graceful shutdowns in Go. I believe this would be beneficial to users of Nebula-go, especially in production environments where resource management is critical.

If upgrading to fbthrift 3.x is not an option at this time, would you be open to discussing potential alternatives or a middle-ground solution to incorporate context support without a full upgrade?

Looking forward to your thoughts.

[Nicole00]

I understand the concern regarding the replace fbthrift 3.x due to given the significant changes it introduces. My goal with this PR was to address the lack of ctx.Context support, which is crucial for request cancellation and graceful shutdowns in Go. I believe this would be beneficial to users of Nebula-go, especially in production environments where resource management is critical.

If upgrading to fbthrift 3.x is not an option at this time, would you be open to discussing potential alternatives or a middle-ground solution to incorporate context support without a full upgrade?

Looking forward to your thoughts.

Sorry for missing this comment. I total agree that the feature of ctx.Context would be beneficial to users, maybe we can provide a new branch for new thrift.

[Nicole00]

@egasimov Hi, I have changed the base branch to go-ctx, could you please fix the conflict when you are free?
Thanks a lot for your great contribution for ctx.Context.

[haoxins]

@egasimov Hi, I have changed the base branch to go-ctx, could you please fix the conflict when you are free? Thanks a lot for your great contribution for ctx.Context.

Wow, this is a great move forward. I've also been waiting for this for a while.

[Nicole00]

@egasimov Hi, I have changed the base branch to go-ctx, could you please fix the conflict when you are free? Thanks a lot for your great contribution for ctx.Context.

Wow, this is a great move forward. I've also been waiting for this for a while.

Thank you both very much for your attention and contributions to the go sdk.

[egasimov]

Hey @Nicole00 , That's good to hear! I have resolved the conflicts and added a few test cases for checking context cancellation. Feel free to review.