armeria icon indicating copy to clipboard operation
armeria copied to clipboard

Support Thrift 0.19

Open minwoox opened this issue 1 year ago • 3 comments

https://github.com/apache/thrift/releases/tag/v0.19.0

minwoox avatar Oct 17 '23 07:10 minwoox

I'm looking this issue. 👀

yeojin-dev avatar Apr 19 '24 06:04 yeojin-dev

Thrift as many know is a RPC framework. Armeria supports thrift on top of its HTTP stack, and basic usage can be found by following the tutorial.

Thrift is known to make quite a few breaking changes in its Java API, so we maintain multiple thrift versions. An example of such a pull request can be found here.

In terms of this issue...

The easiest way to handle this issue is probably empirically.

The way I would handle this is

  1. Create a new module under the thrift directory
  • https://github.com/line/armeria/tree/main/thrift
  1. Copy over the build.gradle file for 0.18.1
  2. We actually need to add the module and dependency in the following files
  • https://github.com/line/armeria/blob/main/settings.gradle
  • https://github.com/line/armeria/blob/main/dependencies.toml
  1. Afterwards, keep running ./gradlew :thrift0.19:clean :thrift0.19:build and handle the failing tests one by one.

One of the most difficult parts of this issue is probably the change to use Apache HttpClient 5 and Jakarta EE. ref: https://github.com/apache/thrift/pull/2746 We probably need to use jetty 11 and also apache httpclient 5 dependencies for the new module. (We possibly need to also add a logback 14 dependency.) We probably also need to fix a couple of tests to use the new APIs.

Ideally, we may try to create a common method so that we don't have to copy over the test files. However, I think it's fine to just make exclusions and copy over the test files if things become too messy.

e.g. We can see some tests are excluded for the thrift0.18 module here.

I would say the issue overall can be considered complete if:

  • Nearly the same test coverage is being applied to the new thrift0.19 module
  • The build passes overall
  • The new tests are not too-invasive if possible (meaning we don't blindly copy tests if we can help it)

Lastly, this would be helpful for many LINE server teams as internally we are using thrift heavily.

jrhee17 avatar Apr 21 '24 15:04 jrhee17

Thrift 0.20.0 is also released. https://mvnrepository.com/artifact/org.apache.thrift/libthrift/0.20.0 @yeojin-dev Any progress? 😉

minwoox avatar May 23 '24 07:05 minwoox

Fixed by #5822

ikhoon avatar Jul 25 '24 06:07 ikhoon