-
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
Rockset data source improvements #4076
Conversation
@arikfr we'd appreciate it if someone can please take a look at this! :) |
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.
Thanks, @ari-e ! See comment.
columns = list(set(map(lambda x: x['field'][0], describe['results']))) | ||
schema[table_name] = {'name': table_name, 'columns': columns} | ||
return schema.values() | ||
pool = ThreadPool(processes=10) |
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.
In some configurations (and this is going to become the default) we run Redash's API server with gevent's monkeypatching. I'm not sure how "healthy" it is to mix threads with gevent.
How about we land this PR without the threads optimization and then revisit this in a follow up?
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.
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.
At the moment the default deployment doesn't use gevent and we don't have a timeline for the switch. So for now I suggest we don't use anything that depends on it.
@arikfr thoughts on ^? |
@arikfr would appreciate some direction here, thanks. |
Abandoning in favor of a simpler PR coming later today |
What type of PR is this? (check all applicable)
Description
This updates the Rockset data source in 2 important ways
commons
workspace was loaded and others were ignoredRelated Tickets & Documents
N/A
Mobile & Desktop Screenshots/Recordings (if there are UI changes)
N/A