-
Notifications
You must be signed in to change notification settings - Fork 930
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.
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
[KYUUBI #5918] Kyuubi BeeLine should check the relocated TTransportException #5919
Conversation
This should be caused by #5783. The current way of extending beeline is fragile, it's really hard to maintain the transitive deps, I suppose we need to rework the kyuubi-beeline module as we did in kyuubi-hive-jdbc and get rid of hive deps eventually. |
kyuubi-hive-beeline/pom.xml
Outdated
@@ -46,6 +46,11 @@ | |||
<version>${project.version}</version> | |||
</dependency> | |||
|
|||
<dependency> | |||
<groupId>org.apache.thrift</groupId> | |||
<artifactId>libthrift</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beeline currently does not have a shade, and the code also has some thrift-related classes.
If the server returns the thrift exception class of the shade, beeline may not be able to handle it correctly.
org.apache.hive.beeline.BeeLine#handleSQLException
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, strictly speaking, we must fork and get rid of vanilla beeline to fix it thoroughly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds like we have to be compatible with two thrift exceptions, one from hiveserver2 and one from kyuubi server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so,we should override BeeLine's handleSQLException method,makes it compatible ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's try this way, the method visibility is package level, we can overwrite it in sub-class located at the same package
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #5919 +/- ##
============================================
- Coverage 61.61% 61.49% -0.13%
Complexity 23 23
============================================
Files 616 616
Lines 36387 36417 +30
Branches 4978 4987 +9
============================================
- Hits 22419 22393 -26
- Misses 11557 11591 +34
- Partials 2411 2433 +22 ☔ View full report in Codecov by Sentry. |
…r/web-ui Bumps [axios](https://github.com/axios/axios) from 0.27.2 to 1.6.0. <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/axios/axios/releases">axios's releases</a>.</em></p> <blockquote> <h2>Release v1.6.0</h2> <h2>Release notes:</h2> <h3>Bug Fixes</h3> <ul> <li><strong>CSRF:</strong> fixed CSRF vulnerability CVE-2023-45857 (<a href="https://redirect.github.com/axios/axios/issues/6028">#6028</a>) (<a href="https://github.com/axios/axios/commit/96ee232bd3ee4de2e657333d4d2191cd389e14d0">96ee232</a>)</li> <li><strong>dns:</strong> fixed lookup function decorator to work properly in node v20; (<a href="https://redirect.github.com/axios/axios/issues/6011">#6011</a>) (<a href="https://github.com/axios/axios/commit/5aaff532a6b820bb9ab6a8cd0f77131b47e2adb8">5aaff53</a>)</li> <li><strong>types:</strong> fix AxiosHeaders types; (<a href="https://redirect.github.com/axios/axios/issues/5931">#5931</a>) (<a href="https://github.com/axios/axios/commit/a1c8ad008b3c13d53e135bbd0862587fb9d3fc09">a1c8ad0</a>)</li> </ul> <h3>PRs</h3> <ul> <li>CVE 2023 45857 ( <a href="https://api.github.com/repos/axios/axios/pulls/6028">#6028</a> )</li> </ul> <pre><code>⚠️ Critical vulnerability fix. See https://security.snyk.io/vuln/SNYK-JS-AXIOS-6032459 </code></pre> <h3>Contributors to this release</h3> <ul> <li> <a href="https://github.com/DigitalBrainJS" title="+449/-114 ([apache#6032](axios/axios#6032) [apache#6021](axios/axios#6021) [apache#6011](axios/axios#6011) [apache#5932](axios/axios#5932) [apache#5931](axios/axios#5931) )">Dmitriy Mozgovoy</a></li> <li> <a href="https://github.com/valentin-panov" title="+4/-4 ([apache#6028](axios/axios#6028) )">Valentin Panov</a></li> <li> <a href="https://github.com/therealrinku" title="+1/-1 ([apache#5889](axios/axios#5889) )">Rinku Chaudhari</a></li> </ul> <h2>Release v1.5.1</h2> <h2>Release notes:</h2> <h3>Bug Fixes</h3> <ul> <li><strong>adapters:</strong> improved adapters loading logic to have clear error messages; (<a href="https://redirect.github.com/axios/axios/issues/5919">#5919</a>) (<a href="https://github.com/axios/axios/commit/e4107797a7a1376f6209fbecfbbce73d3faa7859">e410779</a>)</li> <li><strong>formdata:</strong> fixed automatic addition of the <code>Content-Type</code> header for FormData in non-browser environments; (<a href="https://redirect.github.com/axios/axios/issues/5917">#5917</a>) (<a href="https://github.com/axios/axios/commit/bc9af51b1886d1b3529617702f2a21a6c0ed5d92">bc9af51</a>)</li> <li><strong>headers:</strong> allow <code>content-encoding</code> header to handle case-insensitive values (<a href="https://redirect.github.com/axios/axios/issues/5890">#5890</a>) (<a href="https://redirect.github.com/axios/axios/issues/5892">#5892</a>) (<a href="https://github.com/axios/axios/commit/4c89f25196525e90a6e75eda9cb31ae0a2e18acd">4c89f25</a>)</li> <li><strong>types:</strong> removed duplicated code (<a href="https://github.com/axios/axios/commit/9e6205630e1c9cf863adf141c0edb9e6d8d4b149">9e62056</a>)</li> </ul> <h3>Contributors to this release</h3> <ul> <li> <a href="https://github.com/DigitalBrainJS" title="+89/-18 ([apache#5919](axios/axios#5919) [apache#5917](axios/axios#5917) )">Dmitriy Mozgovoy</a></li> <li> <a href="https://github.com/DavidJDallas" title="+11/-5 ()">David Dallas</a></li> <li> <a href="https://github.com/fb-sean" title="+2/-8 ()">Sean Sattler</a></li> <li> <a href="https://github.com/0o001" title="+4/-4 ()">Mustafa Ateş Uzun</a></li> <li> <a href="https://github.com/sfc-gh-pmotacki" title="+2/-1 ([apache#5892](axios/axios#5892) )">Przemyslaw Motacki</a></li> <li> <a href="https://github.com/Cadienvan" title="+1/-1 ()">Michael Di Prisco</a></li> </ul> <h2>Release v1.5.0</h2> <h2>Release notes:</h2> <h3>Bug Fixes</h3> <ul> <li><strong>adapter:</strong> make adapter loading error more clear by using platform-specific adapters explicitly (<a href="https://redirect.github.com/axios/axios/issues/5837">#5837</a>) (<a href="https://github.com/axios/axios/commit/9a414bb6c81796a95c6c7fe668637825458e8b6d">9a414bb</a>)</li> <li><strong>dns:</strong> fixed <code>cacheable-lookup</code> integration; (<a href="https://redirect.github.com/axios/axios/issues/5836">#5836</a>) (<a href="https://github.com/axios/axios/commit/b3e327dcc9277bdce34c7ef57beedf644b00d628">b3e327d</a>)</li> <li><strong>headers:</strong> added support for setting header names that overlap with class methods; (<a href="https://redirect.github.com/axios/axios/issues/5831">#5831</a>) (<a href="https://github.com/axios/axios/commit/d8b4ca0ea5f2f05efa4edfe1e7684593f9f68273">d8b4ca0</a>)</li> <li><strong>headers:</strong> fixed common Content-Type header merging; (<a href="https://redirect.github.com/axios/axios/issues/5832">#5832</a>) (<a href="https://github.com/axios/axios/commit/8fda2766b1e6bcb72c3fabc146223083ef13ce17">8fda276</a>)</li> </ul> <h3>Features</h3> </blockquote> <p>... (truncated)</p> </details> <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/axios/axios/blob/v1.x/CHANGELOG.md">axios's changelog</a>.</em></p> <blockquote> <h1><a href="https://github.com/axios/axios/compare/v1.5.1...v1.6.0">1.6.0</a> (2023-10-26)</h1> <h3>Bug Fixes</h3> <ul> <li><strong>CSRF:</strong> fixed CSRF vulnerability CVE-2023-45857 (<a href="https://redirect.github.com/axios/axios/issues/6028">#6028</a>) (<a href="https://github.com/axios/axios/commit/96ee232bd3ee4de2e657333d4d2191cd389e14d0">96ee232</a>)</li> <li><strong>dns:</strong> fixed lookup function decorator to work properly in node v20; (<a href="https://redirect.github.com/axios/axios/issues/6011">#6011</a>) (<a href="https://github.com/axios/axios/commit/5aaff532a6b820bb9ab6a8cd0f77131b47e2adb8">5aaff53</a>)</li> <li><strong>types:</strong> fix AxiosHeaders types; (<a href="https://redirect.github.com/axios/axios/issues/5931">#5931</a>) (<a href="https://github.com/axios/axios/commit/a1c8ad008b3c13d53e135bbd0862587fb9d3fc09">a1c8ad0</a>)</li> </ul> <h3>PRs</h3> <ul> <li>CVE 2023 45857 ( <a href="https://api.github.com/repos/axios/axios/pulls/6028">#6028</a> )</li> </ul> <pre><code>⚠️ Critical vulnerability fix. See https://security.snyk.io/vuln/SNYK-JS-AXIOS-6032459 </code></pre> <h3>Contributors to this release</h3> <ul> <li> <a href="https://github.com/DigitalBrainJS" title="+449/-114 ([apache#6032](axios/axios#6032) [apache#6021](axios/axios#6021) [apache#6011](axios/axios#6011) [apache#5932](axios/axios#5932) [apache#5931](axios/axios#5931) )">Dmitriy Mozgovoy</a></li> <li> <a href="https://github.com/valentin-panov" title="+4/-4 ([apache#6028](axios/axios#6028) )">Valentin Panov</a></li> <li> <a href="https://github.com/therealrinku" title="+1/-1 ([apache#5889](axios/axios#5889) )">Rinku Chaudhari</a></li> </ul> <h2><a href="https://github.com/axios/axios/compare/v1.5.0...v1.5.1">1.5.1</a> (2023-09-26)</h2> <h3>Bug Fixes</h3> <ul> <li><strong>adapters:</strong> improved adapters loading logic to have clear error messages; (<a href="https://redirect.github.com/axios/axios/issues/5919">#5919</a>) (<a href="https://github.com/axios/axios/commit/e4107797a7a1376f6209fbecfbbce73d3faa7859">e410779</a>)</li> <li><strong>formdata:</strong> fixed automatic addition of the <code>Content-Type</code> header for FormData in non-browser environments; (<a href="https://redirect.github.com/axios/axios/issues/5917">#5917</a>) (<a href="https://github.com/axios/axios/commit/bc9af51b1886d1b3529617702f2a21a6c0ed5d92">bc9af51</a>)</li> <li><strong>headers:</strong> allow <code>content-encoding</code> header to handle case-insensitive values (<a href="https://redirect.github.com/axios/axios/issues/5890">#5890</a>) (<a href="https://redirect.github.com/axios/axios/issues/5892">#5892</a>) (<a href="https://github.com/axios/axios/commit/4c89f25196525e90a6e75eda9cb31ae0a2e18acd">4c89f25</a>)</li> <li><strong>types:</strong> removed duplicated code (<a href="https://github.com/axios/axios/commit/9e6205630e1c9cf863adf141c0edb9e6d8d4b149">9e62056</a>)</li> </ul> <h3>Contributors to this release</h3> <ul> <li> <a href="https://github.com/DigitalBrainJS" title="+89/-18 ([apache#5919](axios/axios#5919) [apache#5917](axios/axios#5917) )">Dmitriy Mozgovoy</a></li> <li> <a href="https://github.com/DavidJDallas" title="+11/-5 ()">David Dallas</a></li> <li> <a href="https://github.com/fb-sean" title="+2/-8 ()">Sean Sattler</a></li> <li> <a href="https://github.com/0o001" title="+4/-4 ()">Mustafa Ateş Uzun</a></li> <li> <a href="https://github.com/sfc-gh-pmotacki" title="+2/-1 ([apache#5892](axios/axios#5892) )">Przemyslaw Motacki</a></li> <li> <a href="https://github.com/Cadienvan" title="+1/-1 ()">Michael Di Prisco</a></li> </ul> <h3>PRs</h3> <ul> <li>CVE 2023 45857 ( <a href="https://api.github.com/repos/axios/axios/pulls/6028">#6028</a> )</li> </ul> <pre><code>⚠️ Critical vulnerability fix. See https://security.snyk.io/vuln/SNYK-JS-AXIOS-6032459 </code></pre> <h1><a href="https://github.com/axios/axios/compare/v1.4.0...v1.5.0">1.5.0</a> (2023-08-26)</h1> </blockquote> <p>... (truncated)</p> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/axios/axios/commit/f7adacdbaa569281253c8cfc623ad3f4dc909c60"><code>f7adacd</code></a> chore(release): v1.6.0 (<a href="https://redirect.github.com/axios/axios/issues/6031">#6031</a>)</li> <li><a href="https://github.com/axios/axios/commit/9917e67cbb6c157382863bad8c741de58e3f3c2b"><code>9917e67</code></a> chore(ci): fix release-it arg; (<a href="https://redirect.github.com/axios/axios/issues/6032">#6032</a>)</li> <li><a href="https://github.com/axios/axios/commit/96ee232bd3ee4de2e657333d4d2191cd389e14d0"><code>96ee232</code></a> fix(CSRF): fixed CSRF vulnerability CVE-2023-45857 (<a href="https://redirect.github.com/axios/axios/issues/6028">#6028</a>)</li> <li><a href="https://github.com/axios/axios/commit/7d45ab2e2ad6e59f5475e39afd4b286b1f393fc0"><code>7d45ab2</code></a> chore(tests): fixed tests to pass in node v19 and v20 with <code>keep-alive</code> enabl...</li> <li><a href="https://github.com/axios/axios/commit/5aaff532a6b820bb9ab6a8cd0f77131b47e2adb8"><code>5aaff53</code></a> fix(dns): fixed lookup function decorator to work properly in node v20; (<a href="https://redirect.github.com/axios/axios/issues/6011">#6011</a>)</li> <li><a href="https://github.com/axios/axios/commit/a48a63ad823fc20e5a6a705f05f09842ca49f48c"><code>a48a63a</code></a> chore(docs): added AxiosHeaders docs; (<a href="https://redirect.github.com/axios/axios/issues/5932">#5932</a>)</li> <li><a href="https://github.com/axios/axios/commit/a1c8ad008b3c13d53e135bbd0862587fb9d3fc09"><code>a1c8ad0</code></a> fix(types): fix AxiosHeaders types; (<a href="https://redirect.github.com/axios/axios/issues/5931">#5931</a>)</li> <li><a href="https://github.com/axios/axios/commit/2ac731d60545ba5c4202c25fd2e732ddd8297d82"><code>2ac731d</code></a> chore(docs): update readme.md (<a href="https://redirect.github.com/axios/axios/issues/5889">#5889</a>)</li> <li><a href="https://github.com/axios/axios/commit/88fb52b5fad7aabab0532e7ad086c5f1b0178905"><code>88fb52b</code></a> chore(release): v1.5.1 (<a href="https://redirect.github.com/axios/axios/issues/5920">#5920</a>)</li> <li><a href="https://github.com/axios/axios/commit/e4107797a7a1376f6209fbecfbbce73d3faa7859"><code>e410779</code></a> fix(adapters): improved adapters loading logic to have clear error messages; ...</li> <li>Additional commits viewable in <a href="https://github.com/axios/axios/compare/v0.27.2...v1.6.0">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=axios&package-manager=npm_and_yarn&previous-version=0.27.2&new-version=1.6.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `dependabot rebase`. [//]: # (dependabot-automerge-start) Dependabot will merge this PR once CI passes on it, as requested by yaooqinn. [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `dependabot rebase` will rebase this PR - `dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `dependabot merge` will merge this PR after your CI passes on it - `dependabot squash and merge` will squash and merge this PR after your CI passes on it - `dependabot cancel merge` will cancel a previously requested merge and block automerging - `dependabot reopen` will reopen this PR if it is closed - `dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) You can disable automated security fix PRs for this repo from the [Security Alerts page](https://github.com/apache/kyuubi/network/alerts). </details> Closes apache#5671 from dependabot[bot]/dependabot/npm_and_yarn/kyuubi-server/web-ui/axios-1.6.0. Closes apache#5671 243d0c6 [Cheng Pan] license 2fa3217 [Cheng Pan] nit c21a873 [Cheng Pan] nit ae46d5d [Cheng Pan] Update kyuubi-server/web-ui/pnpm-lock.yaml 4adccd2 [dependabot[bot]] Bump axios from 0.27.2 to 1.6.0 in /kyuubi-server/web-ui Lead-authored-by: Cheng Pan <chengpan@apache.org> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Cheng Pan <chengpan@apache.org>
…lang.NoClassDefFoundError: org/apache/thrift/transport/TTransportException
8204e6e
to
ee96c44
Compare
I think the current fix is valid, @cxzl25 do you have any more concerns? |
error(loc("hs2-unknown-connection-problem")); | ||
break; | ||
default: | ||
error(loc("hs2-unexpected-error")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is Hiveserver2 connected by kyuubi beeline, the exception returned by Hiveserver2 is org.apache.thrift.transport.TTransportException
.
It should not be matched here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My description here is wrong, please ignore it.
Thank you for your contribution!
LGTM
Thanks, merged to master |
…portException # 🔍 Description ## Issue References 🔗 This pull request fixes apache#5918, when Kyuubi beeline executes incorrect SQL,it will exit with exception: ``` java.lang.NoClassDefFoundError: org/apache/thrift/transport/TTransportException ``` ## Describe Your Solution 🔧 1.Supply `org.apache.thrift:libthrift` to `kyuubi-beeline` module. 2.We should override BeeLine's handleSQLException method,makes it compatible As we discussed, only `org.apache.kyuubi.shaded.thrift.transport.TTransportException` will be received by `kyuubi-beeline`,so it‘s unnecessary to add libthrift dependency to `kyuubi-beeline`. ## Types of changes 🔖 - [x] Bugfix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Test Plan 🧪 #### Behavior Without This Pull Request ⚰️ #### Behavior With This Pull Request 🎉 #### Related Unit Tests --- # Checklists ## 📝 Author Self Checklist - [ ] My code follows the [style guidelines](https://kyuubi.readthedocs.io/en/master/contributing/code/style.html) of this project - [ ] I have performed a self-review - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [ ] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html) ## 📝 Committer Pre-Merge Checklist - [x] Pull request title is okay. - [x] No license issues. - [x] Milestone correctly set? - [ ] Test coverage is ok - [x] Assignees are selected. - [x] Minimum number of approvals - [x] No changes are requested **Be nice. Be informative.** Closes apache#5919 from hadoopkandy/KYUUBI-5918. Closes apache#5918 ee96c44 [kandy01.wang] [KYUUBI apache#5918] [Bug] kyuubi beeline exit with exception : java.lang.NoClassDefFoundError: org/apache/thrift/transport/TTransportException Authored-by: kandy01.wang <kandy01.wang@vipshop.com> Signed-off-by: Cheng Pan <chengpan@apache.org>
🔍 Description
Issue References 🔗
This pull request fixes #5918, when Kyuubi beeline executes incorrect SQL,it will exit with exception:
Describe Your Solution 🔧
1.Supply
org.apache.thrift:libthrift
tokyuubi-beeline
module.2.We should override BeeLine's handleSQLException method,makes it compatible
As we discussed, only
org.apache.kyuubi.shaded.thrift.transport.TTransportException
will be received bykyuubi-beeline
,so it‘s unnecessary to add libthrift dependency tokyuubi-beeline
.Types of changes 🔖
Test Plan 🧪
Behavior Without This Pull Request ⚰️
Behavior With This Pull Request 🎉
Related Unit Tests
Checklists
📝 Author Self Checklist
📝 Committer Pre-Merge Checklist
Be nice. Be informative.