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

Proper error handling in Hive Queries #4428

Merged
merged 6 commits into from
May 29, 2018

Conversation

maver1ck
Copy link
Contributor

Resolves: #4414
Fetch_many gives strange exception when hive query is in Error state.
So we're reading status to get proper error message before calling fetch_many

@maver1ck
Copy link
Contributor Author

Can we retest this ? Failed check has nothing to do with my patch.

@john-bodley
Copy link
Member

@maver1ck to force restart the Travis CI you may either i) rebase and re-push, or ii) push an empty commit.

@maver1ck
Copy link
Contributor Author

maver1ck commented Mar 1, 2018

@john-bodley thanks

@maver1ck
Copy link
Contributor Author

ping @mistercrunch

@maver1ck
Copy link
Contributor Author

ping @john-bodley
Who can verify this patch ?

@john-bodley
Copy link
Member

@betodealmeida @mistercrunch would you mind reviewing this PR given your experience with Hive?

Copy link
Member

@mistercrunch mistercrunch left a comment

Choose a reason for hiding this comment

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

Sorry about the delay. Curious as to in what kind of errors were mishandled.

state = cursor.poll()
if state.operationState == ttypes.TOperationState.ERROR_STATE:
raise Exception('Query error', state.errorMessage)
if cls.limit_method == LimitMethod.FETCH_MANY:
Copy link
Member

Choose a reason for hiding this comment

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

Let's call super here instead of repeating the code in the parent class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.
Sorry for delay. I was on the vacations :)

@codecov-io
Copy link

codecov-io commented May 29, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@342180b). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #4428   +/-   ##
=========================================
  Coverage          ?   77.48%           
=========================================
  Files             ?       44           
  Lines             ?     8716           
  Branches          ?        0           
=========================================
  Hits              ?     6754           
  Misses            ?     1962           
  Partials          ?        0
Impacted Files Coverage Δ
superset/db_engine_specs.py 53.36% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 342180b...0cebb9d. Read the comment docs.

@maver1ck
Copy link
Contributor Author

@mistercrunch
WDYT ?

@mistercrunch
Copy link
Member

I was trying to understand whether this conflicts with handle_cursor or not.

@maver1ck
Copy link
Contributor Author

I'm using this code on production few months. So probable there is no conflict.

@mistercrunch mistercrunch merged commit ae50845 into apache:master May 29, 2018
timifasubaa pushed a commit to timifasubaa/incubator-superset that referenced this pull request May 31, 2018
* Proper error handling in Hive Queries

* Change quotes

* Trigger checks

* Adding call to parent class

* Small fix

* Fix in method call
timifasubaa pushed a commit to airbnb/superset-fork that referenced this pull request Jul 25, 2018
* Proper error handling in Hive Queries

* Change quotes

* Trigger checks

* Adding call to parent class

* Small fix

* Fix in method call
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
* Proper error handling in Hive Queries

* Change quotes

* Trigger checks

* Adding call to parent class

* Small fix

* Fix in method call
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.26.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.26.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Query on hive: Expected state FINISHED, but found ERROR
4 participants