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

allowing to specify a custom work group for AWS Athena queries #3592

Merged
merged 2 commits into from
Mar 27, 2019

Conversation

ialeinikov
Copy link
Contributor

@ialeinikov ialeinikov commented Mar 15, 2019

What type of PR is this? (check all applicable)

  • Feature

Description

This change allows specifying a custom AWS Athena work group.

Related Tickets & Documents

https://docs.aws.amazon.com/athena/latest/ug/workgroups.html

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Screen Shot 2019-03-15 at 1 20 57 PM

@ghost ghost added the in progress label Mar 15, 2019
Copy link
Contributor

@rauchy rauchy left a comment

Choose a reason for hiding this comment

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

Thanks @ialeinikov! Could you provide more info on how this was tested on your end?

@@ -78,6 +78,11 @@ def configuration_schema(cls):
'type': 'boolean',
'title': 'Use Glue Data Catalog',
},
'work_group': {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add this to the correct place in order? (it happens now automatically due to the alphabetical order, but who knows what will come next)

Copy link
Member

@arikfr arikfr left a comment

Choose a reason for hiding this comment

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

Thanks! Please see comments.

'work_group': {
'type': 'string',
'title': 'Athena work group',
'default': 'primary'
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be safer/more backward compatible to just not pass anything if no value provided?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the AWS documentation:

By default, each account has a primary workgroup and the default permissions allow all authenticated users access to this workgroup. The primary workgroup cannot be deleted.

@@ -78,6 +78,11 @@ def configuration_schema(cls):
'type': 'boolean',
'title': 'Use Glue Data Catalog',
},
'work_group': {
'type': 'string',
'title': 'Athena work group',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'title': 'Athena work group',
'title': 'Athena Work Group',

@ialeinikov
Copy link
Contributor Author

@rauchy, this was tested on our STG instance of Redash with Athena connections.
I've tested it with providing custom values to the "work group" field (based on our configuration) and without any custom values in that field.

@rauchy
Copy link
Contributor

rauchy commented Mar 18, 2019

@ialeinikov great! Have you given PyAthena a sanity check to make sure nothing breaks? (I'm referring to this change)

@ialeinikov
Copy link
Contributor Author

ialeinikov commented Mar 19, 2019

@rauchy, yes, we run redash on that version in our stg without any issues. Moreover, we have some Airflow workloads which uses the updated version of 1.5.0 without issues.

@arikfr arikfr merged commit 7a7fdf9 into getredash:master Mar 27, 2019
@arikfr
Copy link
Member

arikfr commented Mar 27, 2019

Thanks!

harveyrendell pushed a commit to pushpay/redash that referenced this pull request Nov 14, 2019
…dash#3592)

* allowing to specify a custom work group for AWS Athena queries

* Fixing title + adding correct position in the UI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants