Long running queries 2 #70
Conversation
|
@grafana/aws-plugins @andresmgot Adding my high level comments here this time. For more context, see I think this looks like a great step forward! I wonder though if we could take one more step to make AsyncAWSDatasource more generic so that it's closer to be reusable by other plugins. Basically, I want the following interface type AsyncDB interface {
// DB generic methods
driver.Conn
Ping(ctx context.Context) error
// Async flow
StartQuery(ctx context.Context, query string, args ...interface{}) (string, error)
GetQueryID(ctx context.Context, query string, args ...interface{}) (bool, string, error)
QueryStatus(ctx context.Context, queryID string) (QueryStatus, error)
CancelQuery(ctx context.Context, queryID string) error
GetRows(ctx context.Context, queryID string) (driver.Rows, error)
}...to be changed to: type AsyncDatasource interface {
StartQuery(ctx context.Context, query string, args ...interface{}) (string, error)
GetQueryID(ctx context.Context, query string, args ...interface{}) (bool, string, error)
QueryStatus(ctx context.Context, queryID string) (QueryStatus, error)
CancelQuery(ctx context.Context, queryID string) error
GetRows(ctx context.Context, req backend.DataQuery) (data.Frames, error) {
}There might be things that I'm missing here that is preventing us from doing this refactoring now, but can you investigate if it would be possible for AsyncAWSDatasource to call the methods on AsyncDatasource directly instead of going through sqlds? That doesn't mean we have to completely stop using sqlds now - the redshift datasource could for example implement the If this is possible to do without a complete rewrite of the plugin, we should be able to remove any reference to sqlds in the AsyndAWSDatasource and rename it to |
All this is starting to fade in my mind so disregard if this doesn't make sense. The problem I see with that is that basically you will end up with two backend data sources for the same plugin. In the case of an sync request, it would behave as a |
For me the long term goal is to move away from sqlds. sqlds does indeed do a few things for us (rows 2 frames conversion, macro interpolation etc), but it also locks us into a certain pattern where adding new features such as async queries becomes really painful. For AWS sql plugins, I think it's the fact that sqlds acts as a datasource instance that is preventing us for easily enhancing it and adding new features. I'd prefer to have a sqlds library instead that could provide for example utilities for macro interpolation, types for base sql queries and so on. Had we not had sqlds now, it would be fairly straightforward to implement async query support, and the async query data abstraction could live an a separate go package/npm package to could be reused by any plugin. Talked to @iwysiu and we agreed to stick to the path taken in this PR (and its linked PRs). Once it's shipped to production, we can start looking into ways of moving away from sqlds. I'll take a closer look at the code during the day. Thanks everyone! |
completion.go
Outdated
| @@ -49,7 +49,7 @@ func sendResourceResponse(rw http.ResponseWriter, res []string) { | |||
| } | |||
| } | |||
|
|
|||
| func (ds *sqldatasource) getResources(rtype string) func(rw http.ResponseWriter, req *http.Request) { | |||
| func (ds *SqlDatasource) getResources(rtype string) func(rw http.ResponseWriter, req *http.Request) { | |||
There was a problem hiding this comment.
| func (ds *SqlDatasource) getResources(rtype string) func(rw http.ResponseWriter, req *http.Request) { | |
| func (ds *SQLDatasource) getResources(rtype string) func(rw http.ResponseWriter, req *http.Request) { |
| // query sends the query to the connection and converts the rows to a dataframe. | ||
| func query(ctx context.Context, db Connection, converters []sqlutil.Converter, fillMode *data.FillMissing, query *Query) (data.Frames, error) { | ||
| // QueryDB sends the query to the connection and converts the rows to a dataframe. | ||
| func QueryDB(ctx context.Context, db Connection, converters []sqlutil.Converter, fillMode *data.FillMissing, query *Query, args ...interface{}) (data.Frames, error) { |
There was a problem hiding this comment.
Unfortunately, Query is already the name of the query unmarshaling struct.
Second take at this, to have the async code in aws-sdk