-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat(redash): add parallelism support for ingestion #5061
feat(redash): add parallelism support for ingestion #5061
Conversation
@@ -348,6 +355,10 @@ def error(self, log: logging.Logger, key: str, reason: str) -> None: | |||
self.report.report_failure(key, reason) | |||
log.error(f"{key} => {reason}") | |||
|
|||
def warn(self, log: logging.Logger, key: str, reason: str) -> None: | |||
self.report.report_warning(key, reason) | |||
log.warning(f"{key} => {reason}") |
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.
Can you be more descriptive here or is this how we do other places as well?
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.
Yes this is how we do it in other places too
f"sql-parsing-query-{query_id}-datasource-{data_source_id}", | ||
f"exception {e} in parsing {query}", | ||
) | ||
except Exception: |
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.
Is there a reason we don't print out the actual error?
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 error was moved up. Here we can only find problem when table name is wrong
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.
table name is wrong usually due to sql parsing incorrectly getting intermediate temp tables as actual tables.
self.report.max_page_queries = max_page | ||
chart_exec_pool = ThreadPool(self.config.parallelism) | ||
for response in chart_exec_pool.imap_unordered( | ||
self._process_query_response, range(1, max_page + 1) |
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.
why don't you generate here a range from 1 to min(maxpage+1,self.api_page_limit)
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.
self.api_page_limit
is set to math.inf
which is float
. Cannot turn it into int
. The min
function does not support mixed operand types.
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.
lgtm
Checklist