-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add Async QueryData Support #177
Conversation
Backend code coverage report for PR #177
|
Frontend code coverage report for PR #177
|
ec7935d
to
c1f8555
Compare
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.
Great work on this @kevinwcyu and @iwysiu!
Tested and it seems to be working as expected in most of my redshift dashboards except for the pre-configured dashboard called "Amazon Redshift Privileges". Probably something in my local environment, but probably good if you test it to before merging.
Also, would be good if you could add a few words in the PR description and also make sure to mention in the readme how to enable this new feature and that they have to add a new api action in their IAM policy.
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.
Looks really good! Played around a bit with it locally :)
I noticed that if I clicked the run/stop button before filling in a query it seems to get stuck in a loading state forever? Mind taking a look if we can disable the buttons or something?
I looked at the Explore page, it's a bit weird that there's a run button there. I actually have no idea if we have a good way to detect if we're on the explore page besides looking at the current url? Maybe something to ask around about? That said doesn't feel like a blocking bug to me! Maybe we can make a ticket to revisit it later
return false | ||
} | ||
|
||
func (c *API) GetQueryID(ctx context.Context, query string, args ...interface{}) (bool, string, 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.
so in redshift does this function work ok but in athena it has issues?
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.
didn't run into any limitations with redshift for this.
@@ -5,5 +5,5 @@ import { DataSource } from 'datasource'; | |||
import { QueryEditor } from 'QueryEditor'; | |||
|
|||
export function VariableQueryCodeEditor(props: QueryEditorProps<DataSource, RedshiftQuery, RedshiftDataSourceOptions>) { | |||
return <QueryEditor {...props}></QueryEditor>; | |||
return <QueryEditor {...props} hideRunQueryButtons></QueryEditor>; |
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.
how does this run query button work on the explore page? Are there 2 buttons?
Fixes #158
This PR adds Async Query Data support. This allows queries to be handled over multiple requests instead of a single request. By handling a query over multiple requests (starting, checking its status, and fetching the results) this can help with queries that run for a long time that can potentially timeout. Queries have a unique id that is used to track them between different phases of the query handling flow and this id is useful for preventing queries from being re-run multiple times if it is already running (this can happen if a user refreshes their browser).
To enable, set the
redshiftAsyncQueryDataSupport
feature toggle totrue
. The IAM policy actionsredshift-data:ListStatements
andredshift-data:CancelStatement
must be allowed for the policy that Grafana uses to support the functionality.