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

[Presto] Handle uncaught exception in get_create_view #8304

Merged
merged 1 commit into from
Oct 1, 2019

Conversation

etr2460
Copy link
Member

@etr2460 etr2460 commented Sep 26, 2019

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

Resolves a bug introduced in #8213 where calling get_extra_metadata would result in a 5xx because of an uncaught exception. To fix, i've moved the query execute and all the polling inside the try/except block.

TEST PLAN

select a presto table in sql lab, see the metadata (like latest partition) load successfully

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

REVIEWERS

@betodealmeida @john-bodley @graceguo-supercat @villebro

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

Moving execute and entourage inside try-catch makes sense and LGTM. However, while we're at it.. I know this is slightly out of scope for this PR, but I assume the exceptions being thrown from cursor.poll() and cls.execute() can be made more specific? I checked the source for PyHive, and poll()raises ProgrammingError when there is "no query yet" (https://github.com/dropbox/PyHive/blob/master/pyhive/hive.py#L408), which I assume is what the original PR was catching. So I'm guessing the exception causing the 500 from execute is something else. To avoid catching something that shouldn't be caught here, it would probably be good to be more specific with the exact exception being thrown, and how those are handled.

@john-bodley
Copy link
Member

I agree with @villebro. We currently have pylint warnings disabled on a per file basis (we should go through and systematically remove these) as pylint would complain about catching a broad exception here.

@etr2460
Copy link
Member Author

etr2460 commented Sep 26, 2019

I agree that this isn't an ideal solution, but @betodealmeida is really the one with context here. I'll let him chime in on what he was trying to catch with the Exception and then update the PR accordingly

@etr2460 etr2460 force-pushed the erik-ritter--fix-presto-create-view branch from 139c976 to fb28845 Compare October 1, 2019 16:55
@etr2460
Copy link
Member Author

etr2460 commented Oct 1, 2019

@villebro and @john-bodley I've addressed comments

@etr2460 etr2460 merged commit 8c70803 into apache:master Oct 1, 2019
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.35.0 labels Feb 28, 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 size/XS 🚢 0.35.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants