Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

THRIFT-5237 Implement MAX_MESSAGE_SIZE and consolidate limits into a TConfiguration class(JAVA) #2191

Merged
merged 1 commit into from
Sep 18, 2020
Merged

THRIFT-5237 Implement MAX_MESSAGE_SIZE and consolidate limits into a TConfiguration class(JAVA) #2191

merged 1 commit into from
Sep 18, 2020

Conversation

zeshuai007
Copy link
Member

@zeshuai007 zeshuai007 commented Jun 30, 2020 •
edited
Loading

  • Did you create an Apache Jira ticket? (not required for trivial changes)
  • If a ticket exists: Does your pull request title follow the pattern "THRIFT-5237: describe my issue"?
  • Did you squash your changes to a single commit? (not required, but preferred)
  • Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
  • If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

@zeshuai007 zeshuai007 marked this pull request as draft July 1, 2020 02:03
@zeshuai007 zeshuai007 marked this pull request as ready for review July 8, 2020 08:27
@zeshuai007 zeshuai007 marked this pull request as draft July 8, 2020 08:29
@zeshuai007 zeshuai007 force-pushed the Implements_TConfig branch 3 times, most recently from 5db90a8 to 2d01209 Compare July 23, 2020 12:07
@zeshuai007 zeshuai007 marked this pull request as ready for review July 30, 2020 06:47
@Jens-G Jens-G self-requested a review August 7, 2020 14:57
@Jens-G Jens-G added the rebase needed rebase needed label Aug 31, 2020
@Jens-G
Copy link
Member

Jens-G commented Aug 31, 2020

Could you do a rebase? I'd like to look at this PR.

@zeshuai007 zeshuai007 force-pushed the Implements_TConfig branch 2 times, most recently from d5c7f66 to ff1a8dc Compare September 1, 2020 06:49
@zeshuai007
Copy link
Member Author

Done.

@Jens-G Jens-G removed the rebase needed rebase needed label Sep 3, 2020
@Jens-G
Copy link
Member

Jens-G commented Sep 15, 2020

ping

@Jens-G
Copy link
Member

Jens-G commented Sep 17, 2020

+1 review LGTM, not tested. Merge?

@zeshuai007
Copy link
Member Author

Thank you for reviewing this. I would like to merge it , WDYT ?

@Jens-G
Copy link
Member

Jens-G commented Sep 18, 2020

Do it.

@zeshuai007 zeshuai007 merged commit 1190308 into apache:master Sep 18, 2020
@dcelasun
Copy link
Member

@zeshuai007 any chance you could do something similar for Go?

@schlosna schlosna mentioned this pull request Jul 5, 2021
Merged
@emeroad emeroad mentioned this pull request Oct 25, 2021
Closed
@ctubbsii ctubbsii mentioned this pull request Aug 29, 2023
Closed
@zhouyifan279 zhouyifan279 mentioned this pull request Feb 28, 2024
Closed
3 tasks
pan3793 added a commit to apache/kyuubi-shaded that referenced this pull request Feb 29, 2024
@zhouyifan279 @pan3793
[KYUUBI-SHADED #35] Create a light shaded HiveMetaStoreClient for get…
d31a830 
…ting DelegationToken

### _Why are the changes needed?_

org.apache.thrift:libthrift:0.9.3 has serveral CVEs:
-  CVE-2020-13949 - THRIFT-5237(fixed in 0.14.0) - apache/thrift#2191
-  CVE-2019-0205 - THRIFT-4053(fixed in 0.11.0) - apache/thrift#1371
-  CVE-2018-1320 - THRIFT-4506(fixed in 0.9.3.1)

HiveMetaStoreClient of Hive 2.3.9 and 3.1.3 depends on libthrift:0.9.3 and Kyuubi currently uses it to get HMS delegation token.

As Kyuubi only use HiveMetaStoreClient to get delegation token, we think it is better to create a lightweight HiveMetaStoreClient with only the necessary api so that  we can:
- Decouple Kyuubi's libthrift version from Hive
- Remove unnessary dependencies introduced by vanilla HiveMetaStoreClient

### _How was this patch tested?_
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible

- [x] Add screenshots for manual tests if appropriate
<img width="1483" alt="image" src="https://github.com/apache/kyuubi-shaded/assets/88070094/e3198035-6db4-46b1-a47c-db66cb9a9acb">

- [ ] [Run test](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request

Closes #35 from zhouyifan279/hive-metastore-client.

b1654cb [Cheng Pan] nit
b8ec695 [Cheng Pan] fix IDE
2034d62 [Cheng Pan] migrate to kyuubi shaded zk
1cf969e [zhouyifan279] Rename package
170f2e3 [zhouyifan279] Rename package
fe442fc [zhouyifan279] Rename package
fe6f97e [zhouyifan279] Remove unused MetastoreConf.ConfVars
620e834 [zhouyifan279] draft
993218e [zhouyifan279] draft

Lead-authored-by: zhouyifan279 <zhouyifan279@gmail.com>
Co-authored-by: Cheng Pan <chengpan@apache.org>
Signed-off-by: Cheng Pan <chengpan@apache.org>
@pan3793 pan3793 mentioned this pull request Feb 29, 2024
Closed
3 tasks
pan3793 added a commit to apache/kyuubi-shaded that referenced this pull request Feb 29, 2024
@pan3793
[KYUUBI-SHADED #39] Bump Thrift 0.16.0
f740d02 
### _Why are the changes needed?_

The current Thrift 0.9.3-1 has the following CVEs.

- CVE-2020-13949 - THRIFT-5237(fixed in 0.14.0) - apache/thrift#2191
- CVE-2019-0205 - THRIFT-4053(fixed in 0.11.0) - apache/thrift#1371
- CVE-2018-11798 - only affects NodeJS

We choose to upgrade 0.16.0 because
- has no CVEs reported yet
- the latest Hive 4.0.0-beta1 uses Thrift 0.16.0
- Thrift 0.17.0 ~ 0.18.1 has issues on transitive deps
- Thrift 0.18.0 is built on Java 11, which is not compatible with Java 8
- Thrift 0.19.0 restores support for Java 8, but upgrades Apache Http Client5, it involves additional deps

Also, this PR overrides one class `org.apache.thrift.partial.Validate` to remove dependency of Apache Commons Lang3

### _How was this patch tested?_
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible

- [ ] Add screenshots for manual tests if appropriate

- [ ] [Run test](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request

Closes #39 from pan3793/thrift-0.16.

2b7dd2b [Cheng Pan] Bump Thrift 0.16.0

Authored-by: Cheng Pan <chengpan@apache.org>
Signed-off-by: Cheng Pan <chengpan@apache.org>
rtyley added a commit to guardian/scrooge that referenced this pull request Mar 12, 2024
@rtyley
WIP: Upgrade to libthrift 0.19.0
6766fbe 
Scrooge's `TArrayByteTransport` and `TReusableMemoryTransport` classes both
extend libthrift's abstract `org.apache.thrift.transport.TTransport` class.
That class gained 3 new abstract members with apache/thrift#2191
(September 2020), released with libthrift v0.14.0 (February 2021). Consequently,
both classes currently fail to compile, due to missing implementations for these
methods:

def checkReadBytesAvailable(x$1: Long): Unit = ???
def getConfiguration(): org.apache.thrift.TConfiguration = ???
def updateKnownMessageSize(x$1: Long): Unit = ???

Note that Snyk PR twitter#357 updates only
one of several files that needs to be updated for a libthrift upgrade, and
the most important file is probably `build.sbt`, not `demos/scrooge-maven-demo/pom.xml`.
@rtyley rtyley mentioned this pull request Mar 12, 2024
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jens-G Jens-G Jens-G approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@zeshuai007 @Jens-G @dcelasun