You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2020/11/16 16:08:13 UTC
[GitHub] [accumulo] BukrosSzabolcs opened a new pull request #1780: Upgrade Thrift to 0.13.0
BukrosSzabolcs opened a new pull request #1780:
URL: https://github.com/apache/accumulo/pull/1780
In Apache Thrift all versions up to and including 0.12.0, a server or client may run into an endless loop when feed with specific input data.
Some of the classes Accumulo relied on (ie AutoExpandingBufferWriteTransport) are no longer public so the utility methods had to be rewritten. Unfortunately there is no TTransport implementation with the same functionality AutoExpandingBufferWriteTransport provided so I had to switch to TMemoryBuffer as a substitute. However this can not be reset and a new object have to be created for each serialization which is less then optimal. I considered adding a custom implementation, but the changes in the deserialize method should compensate for the the loss of performance.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] BukrosSzabolcs commented on pull request #1780: Upgrade Thrift to 0.13.0
Posted by GitBox <gi...@apache.org>.
BukrosSzabolcs commented on pull request #1780:
URL: https://github.com/apache/accumulo/pull/1780#issuecomment-921761508
@milleruntime Thank you for the merge!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #1780: Upgrade Thrift to 0.13.0
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #1780:
URL: https://github.com/apache/accumulo/pull/1780#issuecomment-920257484
I was able to get Accumulo to compile with Thrift 0.15.0, with the exception of `TFramedTransport`, as I mentioned earlier. This class is a problem because it is directly tied to a configuration property, `general.server.message.size.max`. I emailed the Thrift user list to see if I could get any suggestions as to a replacement or class with similar behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1780: Upgrade Thrift to 0.13.0
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1780:
URL: https://github.com/apache/accumulo/pull/1780#issuecomment-728213014
Thanks for the PR, @BukrosSzabolcs . Thrift upgrades are always highly disruptive for us, as they involve (at minimum):
1. Updates to pom.xml files
2. Updates to our build scripts to generate and place generated Java code
3. Remove any obsoleted workarounds we've added due to bugs in earlier Thrift releases
4. Regenerate and check in new generated Thrift code (and remove any old stuff) (in a separate commit)
5. Fix / workaround any new bugs caused by changes in Thrift libraries
6. Ensure upgrades work across Accumulo versions that change the Thrift version
7. Test, test, test
I think your PR is a good start, but there's lots more to do to be confident about a Thrift version bump, and I'm not sure whether we're going to want to do that for 2.1 yet or not. When we are ready to update, we will certainly include your changes. However, I wanted to offer an explanation in case you don't see this merged right away.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #1780: Upgrade Thrift to 0.13.0
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #1780:
URL: https://github.com/apache/accumulo/pull/1780#issuecomment-920870992
> I was able to get Accumulo to compile with Thrift 0.15.0, with the exception of `TFramedTransport`, as I mentioned earlier. This class is a problem because it is directly tied to a configuration property, `general.server.message.size.max`. I emailed the Thrift user list to see if I could get any suggestions as to a replacement or class with similar behavior.
It was moved to org.apache.thrift.transport.layered.TFramedTransport so it shouldn't be a problem.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] BukrosSzabolcs commented on pull request #1780: Upgrade Thrift to 0.13.0
Posted by GitBox <gi...@apache.org>.
BukrosSzabolcs commented on pull request #1780:
URL: https://github.com/apache/accumulo/pull/1780#issuecomment-728931878
@ctubbsii I fully understand. Thank you for the explanation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1780: Upgrade Thrift to 0.13.0
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1780:
URL: https://github.com/apache/accumulo/pull/1780#issuecomment-796391614
> There has been some recent discussion about following through with a thrift upgrade. It seems since work has been done on this PR, thrift 14.1 has been released. Does this seem like an improvement we should make at this time?
If somebody does the work required to get us to the newer version, then yes, we should apply it. Such work would supersede this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #1780: Upgrade Thrift to 0.13.0
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #1780:
URL: https://github.com/apache/accumulo/pull/1780#issuecomment-920923695
@BukrosSzabolcs I will go ahead and merge your changes here since they get us up to 0.13.0. Then I will make a PR to take us the rest of the way to 0.15.0
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #1780: Upgrade Thrift to 0.13.0
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #1780:
URL: https://github.com/apache/accumulo/pull/1780#issuecomment-920005309
@BukrosSzabolcs unless you have already done the work, I will take a look at migrating to 0.15.0
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime merged pull request #1780: Upgrade Thrift to 0.13.0
Posted by GitBox <gi...@apache.org>.
milleruntime merged pull request #1780:
URL: https://github.com/apache/accumulo/pull/1780
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #1780: Upgrade Thrift to 0.13.0
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #1780:
URL: https://github.com/apache/accumulo/pull/1780#issuecomment-920066477
I have quickly run into an issue between the versions. The type `TFramedTransport` was dropped in version 0.14.0. We currently use this class to set the frame size in `ThriftUtil` and `TServerUtils`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #1780: Upgrade Thrift to 0.13.0
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #1780:
URL: https://github.com/apache/accumulo/pull/1780#issuecomment-915461189
FYI there is another vulnerability in 0.13.0. So if we are going to upgrade, we will probably want to go to 0.14.0. See https://github.com/apache/accumulo/security/dependabot/pom.xml/org.apache.thrift:libthrift/open
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on pull request #1780: Upgrade Thrift to 0.13.0
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on pull request #1780:
URL: https://github.com/apache/accumulo/pull/1780#issuecomment-796081347
There has been some recent discussion about following through with a thrift upgrade. It seems since work has been done on this PR, thrift 14.1 has been released. Does this seem like an improvement we should make at this time?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on pull request #1780: Upgrade Thrift to 0.13.0
Posted by GitBox <gi...@apache.org>.
EdColeman commented on pull request #1780:
URL: https://github.com/apache/accumulo/pull/1780#issuecomment-919446688
Since this was opened - there have been a couple of thrift release 0.14.2 on 6/17 and more recently 0.15.0 on 9/11.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1780: Upgrade Thrift to 0.13.0
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1780:
URL: https://github.com/apache/accumulo/pull/1780#issuecomment-923515614
Since this change was not a complete fix, #2273 that takes us to 0.15.0 will need to be a blocker, or else we'll need to revert this. This change alone breaks my ability to work on the Thrift improvements I was making for sending table IDs instead of table names over our RPC, because I can't rebuild the project with `-Pthrift` without additional changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org