Skip to content
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 timestream support #1918

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

johansmitsnl
Copy link

@johansmitsnl johansmitsnl commented Mar 15, 2021

See PR #1857 which I used as a base platform.

Todo's:

  • Discover endpoint
  • More intensive testing (it works in my use case)

Comment on lines +592 to +598
[dependencies.rusoto_timestream_query]
optional = true
path = "../rusoto/services/timestream-query"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to add timestream-write here too?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

@@ -805,6 +809,7 @@ all = [
"sts",
"support",
"swf",
"timestream-query",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

input: QueryRequest,
) -> Result<QueryResponse, RusotoError<QueryError>> {
self.update_endpoint().await.map_err(|e| match e {
// Remap any DescribeEndpointError to WriteRecordErrors
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong comment

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed

input: CancelQueryRequest,
) -> Result<CancelQueryResponse, RusotoError<CancelQueryError>> {
self.update_endpoint().await.map_err(|e| match e {
// Remap any DescribeEndpointError to WriteRecordErrors
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong comment

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed

@@ -805,6 +809,7 @@ all = [
"sts",
"support",
"swf",
"timestream-query",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

@johansmitsnl
Copy link
Author

@tiratatp I just pushed a new version with the changes you requested. It is partially the PR from #1857 so it is not all my code.

Copy link

@tiratatp tiratatp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but I haven't tested it though. Could you explain a bit how you test this?

&mut self,
input: QueryRequest,
) -> Result<QueryResponse, RusotoError<QueryError>> {
self.update_endpoint().await.map_err(|e| match e {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may be misunderstanding this, but does this mean we update the endpoint every time we do a call?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is due to the endpoints for queries are dynamic. The same for the writer. Also see here service.json of boto

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the endpoints are dynamic -- but they are meant to be cached for the TTL that comes with them (i.e point 3 here:https://docs.aws.amazon.com/timestream/latest/developerguide/Using-API.endpoint-discovery.describe-endpoints.implementation.html). On a second pass though, we don't actually update the endpoint with each call, only when we find at the time of calling that the expiry for the endpoint has passed. Which seems correct.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is ok then?

@johansmitsnl
Copy link
Author

@tiratatp I'm using it in my own project, I just use the writer part but since I only updated the code to the 0.46 version I assume the other authors who also made the PR for the other parts are using (or did) as well.

I just pushed a new commit since I did some better digging into the code and also generated the new files and tested some more bits.

@johansmitsnl
Copy link
Author

@tiratatp do you need input from my side or are you just busy?

@tiratatp
Copy link

@tiratatp do you need input from my side or are you just busy?

I don't have a write request to this repo. The code looks good to me though.

@johansmitsnl
Copy link
Author

@tiratatp @lasartej is there input needed from my side or is this good for merging?

Copy link

@tiratatp tiratatp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@agrinman
Copy link

agrinman commented Aug 1, 2021

Hi, is there any work/review need before merging? Happy to help if there is. Thanks!

@johansmitsnl
Copy link
Author

Rebased on the new master.

@nevins-b
Copy link

nevins-b commented Sep 3, 2021

@johansmitsnl thanks for this! It looks like you rebased and bumped rusoto_core to 0.47 but timestream still depends on 0.46.

@johansmitsnl
Copy link
Author

@nevins-b I have just updated the version and depends. Hope this can get merged soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants