You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/01/28 21:04:22 UTC
[GitHub] [superset] betodealmeida opened a new pull request #12821: fix: don't close cursor before closing connection
betodealmeida opened a new pull request #12821:
URL: https://github.com/apache/superset/pull/12821
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
When running a statement in SQL Lab, after the query is finished we first close the cursor and then close the connection (using the `closing` context manager). This raises an exception in BigQuery due to a bug (https://github.com/googleapis/python-bigquery/issues/497), and while the exception is captured it pollutes the logs, creating an error entry for every statement executed.
I created a PR fixing the bug upstream (https://github.com/googleapis/python-bigquery/pull/498), but in the meantime we can prevent this from happening by simply not closing the cursor explicitly. This is possible because closing a connection will close all its cursors (the exception in BigQuery happens because it tries to close the already closed cursor).
This means we can replace this:
```python
with closing(engine.raw_connection()) as conn:
with closing(conn.cursor()) as cursor:
cursor.execute(sql)
```
With:
```python
with closing(engine.raw_connection()) as conn:
cursor = conn.cursor()
cursor.execute(sql)
```
With this change, BigQuery will no longer try to close the closed cursor, keeping the logs clean.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
N/A
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
Ran a simple query in BigQuery, and verified that the error message no longer shows up in the logs.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov-io commented on pull request #12821: fix: don't close cursor before closing connection
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #12821:
URL: https://github.com/apache/superset/pull/12821#issuecomment-769418916
# [Codecov](https://codecov.io/gh/apache/superset/pull/12821?src=pr&el=h1) Report
> Merging [#12821](https://codecov.io/gh/apache/superset/pull/12821?src=pr&el=desc) (eb3fb37) into [master](https://codecov.io/gh/apache/superset/commit/365770e7c3326db5f86d275dbd2bb13526359b4f?el=desc) (365770e) will **decrease** coverage by `3.45%`.
> The diff coverage is `89.47%`.
[](https://codecov.io/gh/apache/superset/pull/12821?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12821 +/- ##
==========================================
- Coverage 66.84% 63.38% -3.46%
==========================================
Files 1022 488 -534
Lines 50070 30143 -19927
Branches 4914 0 -4914
==========================================
- Hits 33468 19106 -14362
+ Misses 16478 11037 -5441
+ Partials 124 0 -124
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `?` | |
| javascript | `?` | |
| python | `63.38% <89.47%> (-0.45%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12821?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/sql\_lab.py](https://codecov.io/gh/apache/superset/pull/12821/diff?src=pr&el=tree#diff-c3VwZXJzZXQvc3FsX2xhYi5weQ==) | `80.18% <89.47%> (ø)` | |
| [superset/utils/celery.py](https://codecov.io/gh/apache/superset/pull/12821/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2VsZXJ5LnB5) | `86.20% <0.00%> (-13.80%)` | :arrow_down: |
| [superset/databases/commands/create.py](https://codecov.io/gh/apache/superset/pull/12821/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL2NyZWF0ZS5weQ==) | `83.67% <0.00%> (-8.17%)` | :arrow_down: |
| [superset/databases/commands/update.py](https://codecov.io/gh/apache/superset/pull/12821/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `85.71% <0.00%> (-8.17%)` | :arrow_down: |
| [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/superset/pull/12821/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `84.31% <0.00%> (-6.28%)` | :arrow_down: |
| [superset/db\_engine\_specs/sqlite.py](https://codecov.io/gh/apache/superset/pull/12821/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3NxbGl0ZS5weQ==) | `90.62% <0.00%> (-6.25%)` | :arrow_down: |
| [superset/databases/commands/test\_connection.py](https://codecov.io/gh/apache/superset/pull/12821/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL3Rlc3RfY29ubmVjdGlvbi5weQ==) | `84.78% <0.00%> (-4.35%)` | :arrow_down: |
| [superset/models/core.py](https://codecov.io/gh/apache/superset/pull/12821/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `85.59% <0.00%> (-2.99%)` | :arrow_down: |
| [superset/databases/api.py](https://codecov.io/gh/apache/superset/pull/12821/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2FwaS5weQ==) | `89.49% <0.00%> (-2.53%)` | :arrow_down: |
| [superset/views/core.py](https://codecov.io/gh/apache/superset/pull/12821/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `72.97% <0.00%> (-2.45%)` | :arrow_down: |
| ... and [546 more](https://codecov.io/gh/apache/superset/pull/12821/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12821?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/12821?src=pr&el=footer). Last update [365770e...eb3fb37](https://codecov.io/gh/apache/superset/pull/12821?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida merged pull request #12821: fix: don't close cursor before closing connection
Posted by GitBox <gi...@apache.org>.
betodealmeida merged pull request #12821:
URL: https://github.com/apache/superset/pull/12821
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org