-
Notifications
You must be signed in to change notification settings - Fork 916
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
Improve the compatibility of queryTimeout in more version clients #6787
Conversation
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.
LGTM. Since 2112 was released very early, it is recommended to use the new Kyuubi ID instead of FOLLOWUP.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6787 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 687 687
Lines 42439 42441 +2
Branches 5793 5793
======================================
- Misses 42439 42441 +2 ☔ View full report in Codecov by Sentry. |
done |
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.
LGTM.
And how about extracting a method isHive21OrLower
for getProtocolVersion.getValue <=TProtocolVersion.HIVE_CLI_SERVICE_PROTOCOL_V8.getValue
, which is duplicated with org.apache.kyuubi.operation.ExecuteStatement
Line 127 ?
5f69295
to
9fbe1ac
Compare
Thanks, merged to master |
🔍 Description
Issue References 🔗
This pull request fixes #2112
Describe Your Solution 🔧
Similar to #2113, the query-timeout-thread should verify the Thrift protocol version. For protocol versions <= HIVE_CLI_SERVICE_PROTOCOL_V8, it should convert TIMEDOUT_STATE to CANCELED.
Types of changes 🔖
Test Plan 🧪
Behavior Without This Pull Request ⚰️
Behavior With This Pull Request 🎉
Related Unit Tests
Checklist 📝
Be nice. Be informative.