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 2020/09/09 18:26:44 UTC

[GitHub] [incubator-superset] bkyryliuk opened a new pull request #10819: fix: use nullpool in the celery workers [WIP]

bkyryliuk opened a new pull request #10819:
URL: https://github.com/apache/incubator-superset/pull/10819


   ### SUMMARY
   Fixes https://github.com/apache/incubator-superset/issues/10530
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   
   ### TEST PLAN
   <!--- What steps should be taken to verify the changes -->
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [x] Has associated issue: https://github.com/apache/incubator-superset/issues/10530
   
   


----------------------------------------------------------------
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] [incubator-superset] bkyryliuk commented on pull request #10819: fix: use nullpool in the celery workers [WIP]

Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on pull request #10819:
URL: https://github.com/apache/incubator-superset/pull/10819#issuecomment-689805899


   > Was this confirmed to fix the error? There have been similar errors reported for email alerts, so pretty awesome if this solves these problems completely.
   
   @villebro still iterating on it, will post an update once confirmed


----------------------------------------------------------------
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] [incubator-superset] codecov-commenter commented on pull request #10819: fix: use nullpool in the celery workers [WIP]

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #10819:
URL: https://github.com/apache/incubator-superset/pull/10819#issuecomment-689815567


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10819?src=pr&el=h1) Report
   > Merging [#10819](https://codecov.io/gh/apache/incubator-superset/pull/10819?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/3b4a992861e9c6942b37a162316e2be9d48b380b?el=desc) will **decrease** coverage by `4.16%`.
   > The diff coverage is `71.28%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/10819/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/10819?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #10819      +/-   ##
   ==========================================
   - Coverage   65.40%   61.23%   -4.17%     
   ==========================================
     Files         802      804       +2     
     Lines       37872    37936      +64     
     Branches     3561     3561              
   ==========================================
   - Hits        24770    23232    -1538     
   - Misses      12996    14518    +1522     
   - Partials      106      186      +80     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #cypress | `?` | |
   | #javascript | `61.58% <ø> (-0.07%)` | :arrow_down: |
   | #python | `61.03% <71.28%> (+0.13%)` | :arrow_up: |
   
   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/incubator-superset/pull/10819?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset/cli.py](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY2xpLnB5) | `40.00% <33.33%> (+0.06%)` | :arrow_up: |
   | [superset/tasks/schedules.py](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdGFza3Mvc2NoZWR1bGVzLnB5) | `76.87% <62.29%> (+1.12%)` | :arrow_up: |
   | [superset/utils/celery.py](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2VsZXJ5LnB5) | `85.71% <85.71%> (ø)` | |
   | [superset/sql\_lab.py](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQvc3FsX2xhYi5weQ==) | `80.28% <100.00%> (+1.59%)` | :arrow_up: |
   | [superset/tasks/alerts/observer.py](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdGFza3MvYWxlcnRzL29ic2VydmVyLnB5) | `100.00% <100.00%> (ø)` | |
   | [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/explore/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvQXBwLmpzeA==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/dashboard/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/explore/index.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvaW5kZXguanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/dashboard/index.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9pbmRleC5qc3g=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | ... and [196 more](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10819?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/incubator-superset/pull/10819?src=pr&el=footer). Last update [3b4a992...1036e9c](https://codecov.io/gh/apache/incubator-superset/pull/10819?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] [incubator-superset] bkyryliuk commented on a change in pull request #10819: fix: use nullpool in the celery workers

Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #10819:
URL: https://github.com/apache/incubator-superset/pull/10819#discussion_r486600066



##########
File path: superset/tasks/alerts/observer.py
##########
@@ -20,22 +20,22 @@
 from typing import Optional
 
 import pandas as pd
+from sqlalchemy.orm import Session
 
-from superset import db
 from superset.models.alerts import Alert, SQLObservation
 from superset.sql_parse import ParsedQuery
 
 logger = logging.getLogger("tasks.email_reports")
 
 
-def observe(alert_id: int) -> Optional[str]:
+def observe(alert_id: int, session: Session) -> Optional[str]:

Review comment:
       good point, added comments and issue reference




----------------------------------------------------------------
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] [incubator-superset] codecov-commenter edited a comment on pull request #10819: fix: use nullpool in the celery workers [WIP]

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10819:
URL: https://github.com/apache/incubator-superset/pull/10819#issuecomment-689815567


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10819?src=pr&el=h1) Report
   > Merging [#10819](https://codecov.io/gh/apache/incubator-superset/pull/10819?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/3b4a992861e9c6942b37a162316e2be9d48b380b?el=desc) will **decrease** coverage by `4.16%`.
   > The diff coverage is `71.28%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/10819/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/10819?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #10819      +/-   ##
   ==========================================
   - Coverage   65.40%   61.23%   -4.17%     
   ==========================================
     Files         802      804       +2     
     Lines       37872    37936      +64     
     Branches     3561     3561              
   ==========================================
   - Hits        24770    23232    -1538     
   - Misses      12996    14518    +1522     
   - Partials      106      186      +80     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #cypress | `?` | |
   | #javascript | `61.58% <ø> (-0.07%)` | :arrow_down: |
   | #python | `61.03% <71.28%> (+0.13%)` | :arrow_up: |
   
   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/incubator-superset/pull/10819?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset/cli.py](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY2xpLnB5) | `40.00% <33.33%> (+0.06%)` | :arrow_up: |
   | [superset/tasks/schedules.py](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdGFza3Mvc2NoZWR1bGVzLnB5) | `76.87% <62.29%> (+1.12%)` | :arrow_up: |
   | [superset/utils/celery.py](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2VsZXJ5LnB5) | `85.71% <85.71%> (ø)` | |
   | [superset/sql\_lab.py](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQvc3FsX2xhYi5weQ==) | `80.28% <100.00%> (+1.59%)` | :arrow_up: |
   | [superset/tasks/alerts/observer.py](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdGFza3MvYWxlcnRzL29ic2VydmVyLnB5) | `100.00% <100.00%> (ø)` | |
   | [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/explore/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvQXBwLmpzeA==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/dashboard/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/explore/index.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvaW5kZXguanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/dashboard/index.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9pbmRleC5qc3g=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | ... and [196 more](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10819?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/incubator-superset/pull/10819?src=pr&el=footer). Last update [3b4a992...1036e9c](https://codecov.io/gh/apache/incubator-superset/pull/10819?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] [incubator-superset] bkyryliuk merged pull request #10819: fix: use nullpool in the celery workers

Posted by GitBox <gi...@apache.org>.
bkyryliuk merged pull request #10819:
URL: https://github.com/apache/incubator-superset/pull/10819


   


----------------------------------------------------------------
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] [incubator-superset] bkyryliuk commented on pull request #10819: fix: use nullpool in the celery workers [WIP]

Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on pull request #10819:
URL: https://github.com/apache/incubator-superset/pull/10819#issuecomment-690344784


   > Was this confirmed to fix the error? There have been similar errors reported for email alerts, so pretty awesome if this solves these problems completely.
   
   @villebro I've confirmed that this fix is working for us, I've ran alerts and reports over night with high frequency & the issue has stopped happening


----------------------------------------------------------------
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] [incubator-superset] villebro commented on pull request #10819: fix: use nullpool in the celery workers [WIP]

Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #10819:
URL: https://github.com/apache/incubator-superset/pull/10819#issuecomment-689743205


   Was this confirmed to fix the error? There have been similar errors reported for email alerts, so pretty awesome if this solves these problems completely.


----------------------------------------------------------------
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] [incubator-superset] codecov-commenter edited a comment on pull request #10819: fix: use nullpool in the celery workers [WIP]

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10819:
URL: https://github.com/apache/incubator-superset/pull/10819#issuecomment-689815567


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10819?src=pr&el=h1) Report
   > Merging [#10819](https://codecov.io/gh/apache/incubator-superset/pull/10819?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/3b4a992861e9c6942b37a162316e2be9d48b380b?el=desc) will **decrease** coverage by `4.04%`.
   > The diff coverage is `71.28%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/10819/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/10819?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #10819      +/-   ##
   ==========================================
   - Coverage   65.40%   61.35%   -4.05%     
   ==========================================
     Files         802      804       +2     
     Lines       37872    37936      +64     
     Branches     3561     3561              
   ==========================================
   - Hits        24770    23277    -1493     
   - Misses      12996    14473    +1477     
   - Partials      106      186      +80     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #cypress | `?` | |
   | #javascript | `61.58% <ø> (-0.07%)` | :arrow_down: |
   | #python | `61.22% <71.28%> (+0.31%)` | :arrow_up: |
   
   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/incubator-superset/pull/10819?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset/cli.py](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY2xpLnB5) | `40.00% <33.33%> (+0.06%)` | :arrow_up: |
   | [superset/tasks/schedules.py](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdGFza3Mvc2NoZWR1bGVzLnB5) | `76.87% <62.29%> (+1.12%)` | :arrow_up: |
   | [superset/utils/celery.py](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2VsZXJ5LnB5) | `85.71% <85.71%> (ø)` | |
   | [superset/sql\_lab.py](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQvc3FsX2xhYi5weQ==) | `80.28% <100.00%> (+1.59%)` | :arrow_up: |
   | [superset/tasks/alerts/observer.py](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdGFza3MvYWxlcnRzL29ic2VydmVyLnB5) | `100.00% <100.00%> (ø)` | |
   | [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/explore/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvQXBwLmpzeA==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/dashboard/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/explore/index.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvaW5kZXguanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/dashboard/index.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9pbmRleC5qc3g=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | ... and [201 more](https://codecov.io/gh/apache/incubator-superset/pull/10819/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10819?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/incubator-superset/pull/10819?src=pr&el=footer). Last update [3b4a992...1036e9c](https://codecov.io/gh/apache/incubator-superset/pull/10819?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] [incubator-superset] villebro commented on a change in pull request #10819: fix: use nullpool in the celery workers

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #10819:
URL: https://github.com/apache/incubator-superset/pull/10819#discussion_r486530908



##########
File path: superset/tasks/alerts/observer.py
##########
@@ -20,22 +20,22 @@
 from typing import Optional
 
 import pandas as pd
+from sqlalchemy.orm import Session
 
-from superset import db
 from superset.models.alerts import Alert, SQLObservation
 from superset.sql_parse import ParsedQuery
 
 logger = logging.getLogger("tasks.email_reports")
 
 
-def observe(alert_id: int) -> Optional[str]:
+def observe(alert_id: int, session: Session) -> Optional[str]:

Review comment:
       I think the idea was to mostly remove references to sessions in sigs (there's lots of them scattered around), as they can usually just be picked up from `db`, so it might be a good idea to add a comment here that this really needs to be passed along here.

##########
File path: superset/cli.py
##########
@@ -619,6 +620,11 @@ def alert() -> None:
     from superset.tasks.schedules import schedule_window
 
     click.secho("Processing one alert loop", fg="green")
-    schedule_window(
-        ScheduleType.alert, datetime.now() - timedelta(1000), datetime.now(), 6000
-    )
+    with session_scope(nullpool=True) as session:
+        schedule_window(
+            ScheduleType.alert,
+            datetime.now() - timedelta(1000),
+            datetime.now(),
+            6000,
+            session,
+        )

Review comment:
       Not yours, but I usually like using named arguments in cases like this to make stuff more readable. 6000 what?




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