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.

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 #6252] Upgrade hive-service-rpc 4.0.0 #6262

Closed
wants to merge 8 commits into from

Conversation

zhaohehuhu
Copy link
Contributor

@zhaohehuhu zhaohehuhu commented Apr 7, 2024

🔍 Description

Issue References 🔗

close #6252

Describe Your Solution 🔧

Upgrade hive-service-rpc 4.0.0

Types of changes 🔖

  • 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 🧪

Pass existing tests


Checklist 📝

Be nice. Be informative.

@pan3793 pan3793 changed the title Upgrade hive-service-rpc 4.0.0 [KYUUBI #6252] Upgrade hive-service-rpc 4.0.0 Apr 7, 2024
@@ -594,6 +594,45 @@ abstract class TFrontendService(name: String)
resp
}

override def UploadData(req: TUploadDataReq): TUploadDataResp = {
val resp = new TUploadDataResp()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the Kyuubi's implementation is not fully same as Hive, let's throw UnsupportedOperationException here directly, then we don't need to modify other classes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it.

pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Apr 7, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 58.39%. Comparing base (a632edc) to head (9bdf04e).
Report is 14 commits behind head on master.

❗ Current head 9bdf04e differs from pull request most recent head d7df86d. Consider uploading reports for the commit d7df86d to get more accurate results

Files Patch % Lines
...a/org/apache/kyuubi/service/TFrontendService.scala 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #6262      +/-   ##
============================================
- Coverage     58.48%   58.39%   -0.09%     
  Complexity       24       24              
============================================
  Files           651      651              
  Lines         39513    39515       +2     
  Branches       5441     5441              
============================================
- Hits          23109    23075      -34     
- Misses        13937    13964      +27     
- Partials       2467     2476       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pan3793
Copy link
Member

pan3793 commented Apr 7, 2024

LGTM if CI passed

}

override def DownloadData(req: TDownloadDataReq): TDownloadDataResp = {
throw new UnsupportedOperationException("Method DownloadData has not been implemented.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use KyuubiSQLException.featureNotSupported?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. But I see UnsupportedOperationException is used by many places. Should I replace them all or just for here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, we can use TDownloadDataResp with the error status directly, just like CancelDelegationToken

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zhaohehuhu let's narrow the scope to the newly introduced API. "error status" idea SGTM.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, we can use TDownloadDataResp with the error status directly, just like CancelDelegationToken

this approach is not applicable, because the resp has no status field, thus we can not mark the response as failed

pom.xml Outdated
<enabled>false</enabled>
</snapshots>
<id>kyuubi-shaded-staging-0.4.0-rc0</id>
<url>https://repository.apache.org/content/repositories/orgapachekyuubi-1045</url>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just finished the release of Kyuubi Shaded 0.4.0, Jar will be available on Maven Central soon, the staging repo could be removed now.

@pan3793 pan3793 added this to the v1.9.1 milestone Apr 10, 2024
@pan3793 pan3793 closed this in b9c97dd Apr 10, 2024
pan3793 added a commit that referenced this pull request Apr 10, 2024
# 🔍 Description

## Issue References 🔗

close #6252
## Describe Your Solution 🔧

Upgrade hive-service-rpc 4.0.0

## Types of changes 🔖

- [ ] 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 🧪

Pass existing tests

---

# Checklist 📝

- [x] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html)

**Be nice. Be informative.**

Closes #6262 from zhaohehuhu/dev-0407.

Closes #6252

d7df86d [Cheng Pan] address comments
9bdf04e [hezhao2] delete code
fd7b231 [hezhao2] delete code
d524687 [hezhao2] reformat
88c0044 [hezhao2] throws UnsupportedOperationException for UploadData and DownloadData method in TFrontendService
62c5b89 [Cheng Pan] Update pom.xml
1ac087c [Cheng Pan] Update pom.xml
78bca3a [hezhao2] Upgrade hive-service-rpc 4.0.0

Lead-authored-by: hezhao2 <hezhao2@cisco.com>
Co-authored-by: Cheng Pan <pan3793@gmail.com>
Co-authored-by: Cheng Pan <chengpan@apache.org>
Signed-off-by: Cheng Pan <chengpan@apache.org>
(cherry picked from commit b9c97dd)
Signed-off-by: Cheng Pan <chengpan@apache.org>
@pan3793
Copy link
Member

pan3793 commented Apr 10, 2024

Merged to master/1.9. Also cc @slfan1989

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TASK][EASY] Upgrade hive-service-rpc 4.0.0
6 participants