Update thrift library #249
Conversation
fredex42
force-pushed
the
ag/update-thrift
branch
from
1b3d106 to
2437e54
Compare
|
@fredex42 has published a preview version of this PR with release workflow run #69, based on commit 2437e54: 26.0.0-PREVIEW.agupdate-thrift.2024-07-03T1304.2437e540 Want to make another preview release?Click 'Run workflow' in the GitHub UI, specifying the ag/update-thrift branch, or use the GitHub CLI command: gh workflow run release.yml --ref ag/update-thrift Want to make a full release after this PR is merged?Click 'Run workflow' in the GitHub UI, leaving the branch as the default, or use the GitHub CLI command: gh workflow run release.yml |
|
@fredex42 has published a preview version of this PR with release workflow run #70, based on commit 9b369b3: 26.0.0-PREVIEW.agupdate-thrift.2024-07-04T1149.9b369b30 Want to make another preview release?Click 'Run workflow' in the GitHub UI, specifying the ag/update-thrift branch, or use the GitHub CLI command: gh workflow run release.yml --ref ag/update-thrift Want to make a full release after this PR is merged?Click 'Run workflow' in the GitHub UI, leaving the branch as the default, or use the GitHub CLI command: gh workflow run release.yml |
|
@fredex42 has published a preview version of this PR with release workflow run #71, based on commit 0144ace: 26.0.0-PREVIEW.agupdate-thrift.2024-07-04T1251.0144ace4 Want to make another preview release?Click 'Run workflow' in the GitHub UI, specifying the ag/update-thrift branch, or use the GitHub CLI command: gh workflow run release.yml --ref ag/update-thrift Want to make a full release after this PR is merged?Click 'Run workflow' in the GitHub UI, leaving the branch as the default, or use the GitHub CLI command: gh workflow run release.yml |
|
Hi @fredex42 is there a plan to release this PR any time soon? I'm mostly interested in the libthrift version update |
|
Hi @marjisound thanks for the bump. I don't see why not though it was a while since I did this! Seems to be missing a review so maybe @JustinPinner @jonathonherbert amd/or @rtyley could give it the once-over? Am on Feast now so much further away from this stuff. |
|
IIRC we had some issues with reserved keywords in later thrift versions, but if this version has been integrated and tested successfully then we should be good to go. Of course I could be crossing this problem with something else. Will have to look back at where and when we had issues. |
Thanks @JustinPinner I'm not sure about thrift but I also noticed that the scrooge is banning reserved keywords which caused issue in this #231 @rtyley 's comment: #231 (comment) |
|
My apologies @marjisound, that's probably what I was (mis)remembering. In which case we should be ok with this version. Will approve it now. |
There was a problem hiding this comment.
IIRC we had some issues with reserved keywords in later thrift versions,
I think we're talking about content-api-models Thrift definitions using the field name end, which was reserved in later versions of Thrift, right?
content-api-models/models/src/main/thrift/content/v1.thrift
Lines 393 to 394 in 86752f2
Mitigations we've used against this issue have varied over time:
- using
scroogeDisableStrict, which eventually stopped working - downgrading to old versions of the Thrift library or Scrooge library
I now have the shadowy recollection that the newest versions of Thrift have reversed their decision to make so many words 'reserved' - they decided that reserving all the reserved words from 20 or so different supported programming languages was too much. So now, so long as the language you're using doesn't reserve that word, and it's not on a shorter list of universally reserved words, it should be ok. But I think I only heard that, and I don't know what version of Thrift that came in with.
but if this version has been integrated and tested successfully then we should be good to go.
Just checking - is that the case? @fredex42 made a pre-release artifact for this PR in July 2024 as 26.0.0-PREVIEW.agupdate-thrift.2024-07-04T1251.0144ace4. Note that a different set of changes has already been released as v26.0.0. Was that pre-release artifact tried out?
It might be worth rebasing this PR on latest main, doing a new pre-release and seeing if that compiles/passes tests in a couple of test projects like content-api or whatever.
I could be being over-cautious here, if everyone else is ok with what we've got it may well be fine.
| val thriftVersion = "0.15.0" | ||
| val thriftVersion = "0.20.0" |
There was a problem hiding this comment.
Just to note that the latest version of Thrift is apparently v0.21.0, released September 2024 - but no need to jump to that now, just noting that it exists.
I also feel the need for this caution |
|
Just to add, I see an ominous issue in Ophan: If I understand this correctly, Ophan is deliberately avoiding upgrading to Thrift 0.19.0 because of a Scrooge ENUM encoding issue - and it looks like this upgrade in |
Interestingly, I believe the content-api-models-json subproject is already using v0.19.0 because the 0.15.0 dependency is evicted by fezziwig's 0.19.0 dependency. |
This is a really good point - after checking Ophan's code, it appears that it is already (unintentionally, maybe?!) using libthrift 0.20.0 so we can disregard the Ophan concern for evaluating this PR ✨ |
Updates the thrift library to the most recent (Apr 2024)