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

Skeleton for remote query execution using celery. #759

Closed
wants to merge 15 commits into from
Closed

Skeleton for remote query execution using celery. #759

wants to merge 15 commits into from

Conversation

bkyryliuk
Copy link
Member

@bkyryliuk bkyryliuk commented Jul 13, 2016

Current change adds ability to execute the SQL queries on the celery worker.
More info about celery can be found here:

Worker can be started via command caravel worker
Celery is configured in the config.py and by default it is turned off. (CELERY_CONFIG = None)
Fixed some lint issues in the tests.

Tested:

  1. Manually tested if the query workes fine with the CELERY_CONFIG = None and with
class CeleryConfig(object):
    BROKER_URL = 'sqla+sqlite:///celerydb.sqlite'
    CELERY_IMPORTS = ('caravel.tasks', )
    CELERY_RESULT_BACKEND = 'db+sqlite:///celery_results.sqlite'
    CELERY_ANNOTATIONS = {'tasks.add': {'rate_limit': '10/s'}}
CELERY_CONFIG = CeleryConfig

Task is passed and executed:
screen shot 2016-07-13 at 1 21 17 pm

Query works for both cases:
screen shot 2016-07-13 at 1 21 31 pm

Reviewers:

return content

@celery_app.task
def get_sql_results_async(database_id, sql):
Copy link
Member

Choose a reason for hiding this comment

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

i think you could just have a single function get_sql_results decorated with @celery_app.task, and you can call it with .async where needed (on an alternative endpoint)

@bkyryliuk bkyryliuk mentioned this pull request Jul 27, 2016
5 tasks
@jefffeng jefffeng added airbnb Airbnb related enhancement:request Enhancement request submitted by anyone from the community SQL Lab | Backend labels Jul 28, 2016
c_worker.run(**options)


if config.get('CELERY_CONFIG'):
Copy link
Member

@mistercrunch mistercrunch Aug 2, 2016

Choose a reason for hiding this comment

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

I wouldn't put this in module scope, it will print on import.

Copy link
Member Author

Choose a reason for hiding this comment

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

haven't cleaned up after debugging, sorry.

mistercrunch and others added 2 commits August 2, 2016 21:41
This is really just a mock up written in React to try different
components. It could become scaffolding to build a prototype, or not.
}
return json.dumps(data, default=utils.json_int_dttm_ser, allow_nan=False)
if 'tmp_table' in data:
# TODO(bkyryliuk) implement retrieving the data from tmp table.
Copy link
Member

Choose a reason for hiding this comment

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

That would assume waiting for the table which isn't what we want to do here, we can remove 1302 and 1303

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated todo to be:
# TODO(bkyryliuk): add query id to the response and implement the endpoint to poll the status and results.

eng.execute(sql)
return {
'tmp_table': tmp_table_name,
'success': True
Copy link
Member

Choose a reason for hiding this comment

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

+trailing comma

@coveralls
Copy link

coveralls commented Aug 9, 2016

Coverage Status

Coverage decreased (-0.04%) to 81.435% when pulling 15a23d3 on bkyryliuk:celery into 572c6ee on airbnb:master.

@coveralls
Copy link

coveralls commented Aug 10, 2016

Coverage Status

Coverage decreased (-0.04%) to 81.435% when pulling 15a23d3 on bkyryliuk:celery into 572c6ee on airbnb:master.

@coveralls
Copy link

coveralls commented Aug 10, 2016

Coverage Status

Coverage decreased (-0.04%) to 81.435% when pulling 15a23d3 on bkyryliuk:celery into 572c6ee on airbnb:master.

zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 17, 2021
1. move translation, connection, query to core (apache#729)
2. update encodable to remove formatter inter-dependency (apache#744)
3. move number-format and time-format to core (apache#730)
4. move superset-ui/dimension to core (apache#732)
5. move superset-ui/color to core (apache#755)
6. move superset-ui/style to core (apache#756)
7. move superset-ui/validator to core (apache#757)
8. move superset-ui/chart-composition to core (apache#759)
9. move superset-ui/chart to core (apache#760)
zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 24, 2021
1. move translation, connection, query to core (apache#729)
2. update encodable to remove formatter inter-dependency (apache#744)
3. move number-format and time-format to core (apache#730)
4. move superset-ui/dimension to core (apache#732)
5. move superset-ui/color to core (apache#755)
6. move superset-ui/style to core (apache#756)
7. move superset-ui/validator to core (apache#757)
8. move superset-ui/chart-composition to core (apache#759)
9. move superset-ui/chart to core (apache#760)
zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 25, 2021
1. move translation, connection, query to core (apache#729)
2. update encodable to remove formatter inter-dependency (apache#744)
3. move number-format and time-format to core (apache#730)
4. move superset-ui/dimension to core (apache#732)
5. move superset-ui/color to core (apache#755)
6. move superset-ui/style to core (apache#756)
7. move superset-ui/validator to core (apache#757)
8. move superset-ui/chart-composition to core (apache#759)
9. move superset-ui/chart to core (apache#760)
zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 26, 2021
1. move translation, connection, query to core (apache#729)
2. update encodable to remove formatter inter-dependency (apache#744)
3. move number-format and time-format to core (apache#730)
4. move superset-ui/dimension to core (apache#732)
5. move superset-ui/color to core (apache#755)
6. move superset-ui/style to core (apache#756)
7. move superset-ui/validator to core (apache#757)
8. move superset-ui/chart-composition to core (apache#759)
9. move superset-ui/chart to core (apache#760)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
airbnb Airbnb related enhancement:request Enhancement request submitted by anyone from the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants