Migrate to aws sdk v2 #325
Conversation
| return c.Stop(&api.ExecuteQueryOutput{ID: queryID}) | ||
| } | ||
|
|
||
| func (c *API) Stop(output *api.ExecuteQueryOutput) error { | ||
| _, err := c.DataClient.CancelStatement(&redshiftdataapiservice.CancelStatementInput{ | ||
| _, err := c.DataClient.CancelStatement(context.TODO(), &redshiftdata.CancelStatementInput{ |
There was a problem hiding this comment.
Not in this commit but do you think we should we do a v2 of the async sdk that passes a context?
There was a problem hiding this comment.
I think that's a good idea!
| res = append(res, *sc) | ||
| } | ||
| } | ||
| res = append(res, out.Databases...) |
There was a problem hiding this comment.
I assume we're not worried about nil databases anymore?
There was a problem hiding this comment.
Yep, this is one of quite a lot of places where the sdk v2 returns non-pointer values where v1 returned pointers...
pkg/redshift/driver/rows.go
Outdated
| @@ -183,7 +178,7 @@ func (r *Rows) Close() error { | |||
| func (r *Rows) fetchNextPage(token *string) error { | |||
| var err error | |||
|
|
|||
| r.result, err = r.service.GetStatementResult(&redshiftdataapiservice.GetStatementResultInput{ | |||
| r.result, err = r.service.GetStatementResult(context.TODO(), &redshiftdata.GetStatementResultInput{ | |||
There was a problem hiding this comment.
could we pass a context into fetchNextPage? it looks like at least 1 of the places its called could.
There was a problem hiding this comment.
We could where it's called from newRows but not from what I think is the main user - the Next method on line 43 here, which implements an interface from sql/driver. Although - we could do the not-ideal thing of storing the context on our Rows struct. Since the Rows struct is tied to a particular query ID that's probably not a bad idea here...
pkg/redshift/driver/rows.go
Outdated
| ret[i] = nil | ||
| // FIXME: I think this is the correct translation of the existing behavior but I'm not | ||
| // sure it's actually the correct behavior | ||
| if isNull, ok := curr.(*redshiftdatatypes.FieldMemberIsNull); ok { |
There was a problem hiding this comment.
nit: can you do
| if isNull, ok := curr.(*redshiftdatatypes.FieldMemberIsNull); ok { | |
| if isNull, ok := curr.(*redshiftdataV2types.FieldMemberIsNull); ok && isNull.Value { |
There was a problem hiding this comment.
Looking back, yeah, that is actually a more accurate translation. I'm still not sure it's the right thing to do, but it's at least the samest thing to do :)
njvrzm
force-pushed
the
njvrzm/migrate_to_aws_sdk_v2
branch
from
49b48fa to
ffdcd29
Compare
|
Built it and clicked around a bit and everything looks good! Tried it with access & secret key and assume role datasources Resource calls as well as queries work fine. |
njvrzm
force-pushed
the
njvrzm/migrate_to_aws_sdk_v2
branch
from
5ba4b4a to
d7fd505
Compare
njvrzm
force-pushed
the
njvrzm/migrate_to_aws_sdk_v2
branch
from
7db51ed to
468dd32
Compare
This is a
proof of conceptdemonstration of migrating data sources' direct use of the aws-sdk-go from v1 to v2.I've tested it with one provisioned datasource ("AWS Redshift assume role"). I haven't exercised it thoroughly yet but what I have tried - health check, simple query - does work.