Long running queries 2

[iwysiu]

Second take at this, to have the async code in aws-sdk

[sunker]

@grafana/aws-plugins @andresmgot Adding my high level comments here this time. For more context, see discussion in https://github.com/grafana/sqlds/pull/69.

I think this looks like a great step forward! sqlds is now basically untouched and I appreciate that. Haven't reviewed the code in detail yet, but in general, I think this is something we could ship to production.

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 GetRows(ctx context.Context, req backend.DataQuery) (data.Frames, error) method, and then just call handleQuery in sqlds (this method would have to be exported then).

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 AsyncDatasource.

[andresmgot]

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

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 sqlds while for async requests it would be an AsyncDataSource. Having the abstraction at the database layer allows you to share some logic (db related) between the sync and the async flow. It has the disadvantage of not being able to follow the async flow for things that are not databases but afaik all the cases we are evaluating are databases.

[sunker]

The problem I see with that is that basically you will end up with two backend data sources for the same plugin.

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!