[Feature] Lack of context.Context support #350

egasimov commented 1 week ago

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 commented 1 week ago

All committers have signed the CLA.

Nicole00 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 6 days ago

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

Nicole00 commented 4 days ago

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 commented 3 days ago

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!

vesoft-inc / nebula-go