search

apache / fluo

Apache Fluo
https://fluo.apache.org
Apache License 2.0
187 stars 78 forks source link

Update thrift to 0.16.0 #1111

Closed ctubbsii closed 2 years ago

ctubbsii commented 3 years ago
ctubbsii commented 3 years ago

This is low risk, because libthrift is shaded into Fluo, so there shouldn't be any conflicts with other versions of thrift from Accumulo or elsewhere.

ctubbsii commented 3 years ago

The changes looks good. There were surprisingly few changes in the generated code. The build failed, I restarted to see if it will pass. Were you able to run a full build locally?

No. I'm not sure why it is failing. I thought maybe it had to do with Java 11 issues, but haven't had time to investigate. Fluo shades Thrift, so it shouldn't be a conflict with other dependencies, and the only API breakage was the relocation of TFastFramedTransport to a subpackage. All the tests seem to be hanging/timing out. I'm not sure why.

ctubbsii commented 2 years ago

@keith-turner It only took a year, but I think I figured out the problem with the ITs when the libthrift version was bumped. The name of the property we use in Fluo to manage the version of the shaded libthrift in Fluo is the same name as the property in accumulo-core. So, when resolving the version of libthrift during integration tests, it picked up the wrong version. It needed the version Accumulo uses, but it picked up the version Fluo had configured. This caused a ClassNotFoundException in Accumulo Maven Plugin, and Accumulo couldn't start for the ITs to run.

ctubbsii commented 2 years ago

Merging since this was previously approved, and I haven't made any substantive changes since, other than to fix the ITs running.