-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Run queries through ad-hoc SSH tunnels #4797
Changes from 8 commits
231f19b
93a9748
8da215b
75a48b0
7f3b458
9029e6e
c38d826
bc69c03
6a26d78
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
import logging | ||
import time | ||
|
||
from flask import make_response, request | ||
from flask_restful import abort | ||
|
@@ -20,12 +21,16 @@ | |
) | ||
from redash.utils import filter_none | ||
from redash.utils.configuration import ConfigurationContainer, ValidationError | ||
from redash.tasks.general import test_connection, get_schema | ||
from redash.serializers import serialize_job | ||
|
||
|
||
class DataSourceTypeListResource(BaseResource): | ||
@require_admin | ||
def get(self): | ||
return [q.to_dict() for q in sorted(query_runners.values(), key=lambda q: q.name())] | ||
return [ | ||
q.to_dict() for q in sorted(query_runners.values(), key=lambda q: q.name()) | ||
] | ||
|
||
|
||
class DataSourceResource(BaseResource): | ||
|
@@ -182,19 +187,9 @@ def get(self, data_source_id): | |
require_access(data_source, self.current_user, view_only) | ||
refresh = request.args.get("refresh") is not None | ||
|
||
response = {} | ||
|
||
try: | ||
response["schema"] = data_source.get_schema(refresh) | ||
except NotSupported: | ||
response["error"] = { | ||
"code": 1, | ||
"message": "Data source type does not support retrieving schema", | ||
} | ||
except Exception: | ||
response["error"] = {"code": 2, "message": "Error retrieving schema."} | ||
Comment on lines
-189
to
-195
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to keep reporting these error codes. For example, we might show different UI when it's not supported. And we don't want to throw random errors at the user but rather show "Error retrieving schema." (it might make sense to show some of the errors, but it's beyond the scope of this PR). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought so too, but then I realized we aren't really using the error codes defined in the service. The current implementation (returning There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know that we don't really use them anymore in code -- this is something that happened during the conversion to React, but we should someday. It's better to be explicit about this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
job = get_schema.delay(data_source.id, refresh) | ||
|
||
return response | ||
return serialize_job(job) | ||
|
||
|
||
class DataSourcePauseResource(BaseResource): | ||
|
@@ -245,10 +240,14 @@ def post(self, data_source_id): | |
) | ||
|
||
response = {} | ||
try: | ||
data_source.query_runner.test_connection() | ||
except Exception as e: | ||
response = {"message": str(e), "ok": False} | ||
|
||
job = test_connection.delay(data_source.id) | ||
while not (job.is_finished or job.is_failed): | ||
time.sleep(1) | ||
job.refresh() | ||
|
||
if isinstance(job.result, Exception): | ||
response = {"message": str(job.result), "ok": False} | ||
else: | ||
response = {"message": "success", "ok": True} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -284,24 +284,28 @@ def serialize_job(job): | |
updated_at = 0 | ||
|
||
status = STATUSES[job_status] | ||
query_result_id = None | ||
result = query_result_id = None | ||
|
||
if job.is_cancelled: | ||
error = "Query cancelled by user." | ||
status = 4 | ||
elif isinstance(job.result, Exception): | ||
error = str(job.result) | ||
status = 4 | ||
elif isinstance(job.result, dict) and "error" in job.result: | ||
error = job.result["error"] | ||
status = 4 | ||
else: | ||
error = "" | ||
query_result_id = job.result | ||
result = query_result_id = job.result | ||
|
||
return { | ||
"job": { | ||
"id": job.id, | ||
"updated_at": updated_at, | ||
"status": status, | ||
"error": error, | ||
"result": result, | ||
"query_result_id": query_result_id, | ||
Comment on lines
+308
to
309
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To avoid accumulating tech debt, maybe we can return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (only if it's simple check) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we want to avoid accumulating tech debt, we should probably always return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But this will break scripts that use /jobs for polling and expect There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, that's why I wen't for doubling |
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds a noticeable delay to loading the schema browser without indication that it's loading the schema, could we add a "Loading... please wait" screen or something similar? Or reduce the sleep interval to something that is closer to an unnoticeable perception level?
Also, in case of a worker failure that prevents the job from ever returning a good result (e.g. Redis key eviction), could this add an upper limit on the number of tries to load the data and then show a button to fetch the schema anew with a new job?