support for pagination for timestream.list_databases list_tables

[cnfait]

Feature or Bugfix

• Bugfix

Relates

• Add support for pagination for wr.timestream.list_databases and wr.timestream.list_tables #1822

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[malachi-constant]

AWS CodeBuild CI Report

• CodeBuild project: GitHubCodeBuild8756EF16-4rfo0GHQ0u9a
• Commit ID: f3731cd
• Result: FAILED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[kukushking]

Looks good!

[jaidisido]

Looks good, let's just make sure the CB passes

[LeonLuttenberger]

Do you think it's worthwhile to create some tests for this? Creating too many timestream databases/tables might take too long, but maybe we could just mock the boto calls in order to make sure that we're actually testing the pagination. As in the first response returns nextToken, but the second one does not.

I've written and deployed code in the past where my while "nextToken" in response was never properly tested, and thus ended up not working when we needed to do pagination.

[cnfait]

Do you think it's worthwhile to create some tests for this? Creating too many timestream databases/tables might take too long, but maybe we could just mock the boto calls in order to make sure that we're actually testing the pagination. As in the first response returns nextToken, but the second one does not.

I've written and deployed code in the past where my while "nextToken" in response was never properly tested, and thus ended up not working when we needed to do pagination.

I'm ok updating the tests to check that pagination works properly - but in that case it is better if I handle MaxResults (add a "limit" argument to list_databases/list_tables perhaps?) as well to avoid having to create tens of databases/tables.

Long term I'm actually wondering if it is worth having list_databases and list_tables at all in our API. People can use straight-boto3 and I'm not sure what value we're adding for these two calls. Any thought on this?

[kukushking]

@cnfait My 2 cents: it's things like handling pagination is the value add here so we still might be a bit more convenient than pure boto3 calls. Also btw we could look into using generators and yielding the pages although it's not a very big win memory-wise but still nice to be able to fetch it dynamically.

[cnfait]

@cnfait My 2 cents: it's things like handling pagination is the value add here so we still might be a bit more convenient than pure boto3 calls. Also btw we could look into using generators and yielding the pages although it's not a very big win memory-wise but still nice to be able to fetch it dynamically.

I thought about it but didn't want to change the type signature of the functions. Perhaps it's okay though, or I can do it for 3.0.0?

[kukushking]

@cnfait I guess type signature could be a Union[pd.DataFrame, Iterator[pd.DataFrame]] in which case it stays backward-compatible and the iterator behaviour could be enabled by chunked parameter that is False by default like we do for some other calls - in that case we could even ship it with main but I do not have strong preference here it's fine to ship in with 3.0, too

[kukushking]

Btw all of that is just a wishlist and can be addressed by the following PRs - this one looks great already!

[malachi-constant]

AWS CodeBuild CI Report

• CodeBuild project: GitHubCodeBuild8756EF16-4rfo0GHQ0u9a
• Commit ID: dc9b64e
• Result: FAILED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[malachi-constant]

AWS CodeBuild CI Report

• CodeBuild project: GitHubCodeBuild8756EF16-4rfo0GHQ0u9a
• Commit ID: 739d264
• Result: FAILED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository