Long Running Queries #61
Conversation
|
|
|
|
go.mod
Outdated
| ) | ||
|
|
||
| replace github.com/grafana/sqlds/v2 => ../sqlds |
There was a problem hiding this comment.
This is just a temporary change to be able to test locally.
kevinwcyu
requested review from
a team and
fridgepoet
and removed request for
a team
pkg/awsds/asyncDatasource.go
Outdated
| } | ||
|
|
||
| if driverSettings.Timeout != 0 { | ||
| tctx, cancel := context.WithTimeout(ctx, driverSettings.Timeout) |
There was a problem hiding this comment.
I'm not sure how much sense it makes to make use of the timeout when async mode is enabled. Maybe we should deprecate this feature once the long running queries becomes the default way (or only way) of querying this backend?
There was a problem hiding this comment.
Yeah, it's a little weird because its the time out for individual operations here instead of the whole data retrieval, which is what it means in sync
There was a problem hiding this comment.
Yeah, let's remove it from the async flow then. It will only add confusion.
pkg/awsds/asyncDatasource.go
Outdated
| }, nil | ||
| } | ||
|
|
||
| conn, err := ds.driver.Connect(ds.connSettings, q.ConnectionArgs) |
There was a problem hiding this comment.
Don't we need to use getDBConnectionFromQuery here, like in sqlds?
There was a problem hiding this comment.
You may want to test this manually and make sure this works as expected. You can for example add two query rows in a panel and make sure they use different regions. Then each of the query rows should have one connection each.
There was a problem hiding this comment.
I looked into it, but from what I can tell there aren't configurable query options for redshift that would force different connections.
| *sqlds.SQLDatasource | ||
| asyncDB AsyncDB |
There was a problem hiding this comment.
this is why I think this is not an "async datasource" but an "async sql data source" since we are still tied to the database architecture of sqlds
There was a problem hiding this comment.
That's fair, but I think the naming linking it to AWS is specific enough? Also, for context, we're currently discussing removing sqlds and replacing it with a bunch of utility functions so we aren't as stuck with the sqlds architecture.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| _, err = ds.SQLDatasource.NewDatasource(settings) |
There was a problem hiding this comment.
this is a bit weird, you are creating a data source using sqlds but not using it?
There was a problem hiding this comment.
It initializes the sqlds.SQLDatasource that AsyncAWSDatasource is wrapping. Added a comment
There was a problem hiding this comment.
okay, I think we got the naming there wrong. It should be func (ds *SQLDatasource) Init(settings backend.DataSourceInstanceSettings) (error), also because we already have a NewDatasource that is really generating a new data source.
There was a problem hiding this comment.
Yeah, it's badly named 😞 Unfortunately I don't think the sqlds function can be renamed at this point without a breaking change.
There was a problem hiding this comment.
well, you just released that so nobody is using it but here so you have some flexibility if you want to modify it. In any case releases are free and numbers are infinite so there is no problem doing breaking changes :) (at least for things like this)
There was a problem hiding this comment.
Unfortunately it was already being exported NewDatasource before the long running queries changes 😞 I could deprecate it, but again, if we're planning on just not using it in the future there doesn't seem to be that much of a point.
Part of #64