Fixes #452 - supporting thrift 0.7 and 0.9 #455
Conversation
EugenCepoi
force-pushed
the
master
branch
8 times, most recently
from
efa4cfa
to
bf625df
Compare
| @@ -7,11 +7,16 @@ before_install: | |||
| - sudo apt-get update -qq | |||
There was a problem hiding this comment.
can you change it so these are independent travis builds? it will keep the travis speeds up and isolate failures from one side to the other.
There was a problem hiding this comment.
you mean by using travis matrix builds? will have a look
There was a problem hiding this comment.
Exactly. We use them in things like scalding/summingbird to try avoid single large builds
|
Sorry i've been away so little slow to get to this. Looks pretty much perfect to me, few minor comments only. |
|
Great thanks! About the releasing part. What do you think, should we add classifiers for thrift 7 and 9 and release both? I think it would be nicer for end users so they don't need to fork, build and deploy their specific version. |
|
Yes i think classifiers would be awesome for releasing, can leave the default, but having thrift 9+ artifacts in maven central would make it much easier for new users |
| - mvn test -Dtest.library.path=$PWD/hadoop-lzo-native/lib -Drequire.lzo.tests=true -P hadoop2 | ||
| - mvn test -Dthrift.cmd=/var/thrift-old/bin/thrift -Dtest.library.path=$PWD/hadoop-lzo-native/lib -Drequire.lzo.tests=true -P hadoop2 | ||
| - echo ============ Build and test for thrift 0.9+ ================ | ||
| - mvn clean test -Dthrift.cmd=/var/thrift-new/bin/thrift -Dtest.library.path=$PWD/hadoop-lzo-native/lib -Drequire.lzo.tests=true -P thrift9 |
There was a problem hiding this comment.
Mhhh I think that the build against hadoop X and test against hadoop Y never worked. Running mvn test includes mvn compile. So can I just drop this?
The matrix would include following scenarios: hadoop 1 and thrift 0.7, hadoop 1 and thrift 0.9, hadoop 2 and thrift 0.7, hadoop 2 and thrift 0.9. Or should I remove one of the hadoop X and thrift Y assuming that the thrift code is compatible with any hadoop version?
EugenCepoi
force-pushed
the
master
branch
3 times, most recently
from
1d74c43
to
69b5471
Compare
|
Ok so squashed all that into two commits, first one the feature it self second one the matrix build. |
|
Looks good to me once we get releasing down. Once you've that code in I'll just grab this branch to have a quick sense check with some local releases. Don't see anything else though. Any thoughts @isnotinvain or @rangadi |
|
After being busy with other stuff I finally found the time to give it a shot. @ianoc I've updated the PR with automatic releases of the artifacts build for thrift 7 and 9. This was pretty tricky and I couldn't find anything else working from end to end using the std maven plugins. So I ended up doing some stuff via bash script. I tested it in on a ubuntu vagrant box with releases to a local repo (on the local fs), which worked for me. Let me know if you want me to explain my choices or if I need to update some doc to reflect this new release process. |
|
Something is strange not only the error is surprising but the project versions that I see in the build don't correspond to the actual code... in the travis logs we can see |
|
Ok so the issue was related to travis who is experimenting with google cloud platform which in turn doesn't have an ip address associated to the local hostname which in turn hits some bug in the jvms native libnet.so... I added a fix for it in the build, seems to work... ouf :p The travis issue: travis-ci/travis-ci#1484 |
EugenCepoi
changed the title
| trans.reset(bytes, offset, len); | ||
| base.read(protocol); | ||
| } | ||
|
|
||
| protected void resetAndInitialize(TBinaryProtocol protocol, int newLength) { | ||
| protocol.reset(); |
There was a problem hiding this comment.
Does this mean you are not setting the limits in Thrift 0.9? Is it not useful there?
There was a problem hiding this comment.
If you are talking about setReadLength, then yeah it has been removed from thrift 9.
|
@EugenCepoi thanks for the doing this. I came across this incompatibility today. Would love to get this in. I have one above about not setting length limits for Thrift 9. |
| @@ -0,0 +1,285 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
What does this do? We can avoid having to publish multiple versions of packages for thrift-0.7 & thrift-0.9. This is how we handle multiple hadoop versions (there the incompatibility is much greater). This is how it could work : the build could be either with thrift-0.7 or thrift-0.9 profile, but at runtime, ThriftCompat would detect which version of thrift is available and would handle the length limit appropriately.
This also most mean, we would have to move thrift dependency to 'provided' scope in mvn, which is fine.
|
Please hold off on this, I will have smaller patch (but bit more hacky, essentially read these the values through reflection). I will try to get this out on Saturday... currently busy with something else. |
|
Heh I can't hold off on this as everything is already done :( |
|
I think you can still try this branch out to get a feel of how it works and choose between the different trade-offs. |
Fixes #452 - supporting thrift 0.7 and 0.9
|
Merged, talked with Raghu offline (who suggested I decide). I'm inclined to say this his a high quality contribution and seems like the pro-style most forward looking way to do this. (no runtime surprises). Thanks for all the work in getting this right @EugenCepoi ! |
|
Cool, I am glad if you guys find that it is the right way (hesitated a lot between the different solutions). You should try it out with a dummy release. I tested it but only with local fs repos so maybe there might be some subtleties using sonatypes repo. Thanks. |
|
I had already coded #458 before this was merged. I will close it. |
An attempt to #452 - support thrift 0.7 and 0.9+ by using separate source trees enabled via maven profiles.
By default the enabled profile is thrift7 to remain compatible with the current behaviour/use. Building against thrift 9 can be done by enabling thrift9 profile.
The code that is incompatible has been isolated so most future changes can benefit to both versions.
I have tested it against thrift 0.9.1 and 0.7.0.
What this doesn't handle yet is a dual release...this could be achieved using classifiers. But first I want to have those changes validated before attempting to make sure the release works properly.
@ianoc can you please have a look at this PR.