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

S3 pagination implementation #719

Merged
merged 5 commits into from
Feb 24, 2023
Merged

S3 pagination implementation #719

merged 5 commits into from
Feb 24, 2023

Conversation

mzorko
Copy link
Contributor

@mzorko mzorko commented Feb 22, 2023

Pagination implementation for S3 board of the pin_list.pins_board_s3 & pin_versions.pins_board_s3.

Pagination implementation for S3 board of the `pin_list.pins_board_s3` & `pin_versions.pins_board_s3`.
@juliasilge
Copy link
Member

Thank you so much for this contribution @mzorko! Is there a way to update this PR (or make a new one) to not include the whitespace changes?

@mzorko
Copy link
Contributor Author

mzorko commented Feb 23, 2023

Hi @juliasilge. Sure, sorry for that - there is a new commit included. Please check if it is more transparent this time.

@juliasilge
Copy link
Member

@mzorko This looks great to me. Do you think it's possible to write a test for this that would be straightforward and not too slow? Or would it require writing just a zillion pins (or versions)? You can check out the suites of tests we run across the boards (like this) but this would be an S3-specific test, I think.

@mzorko
Copy link
Contributor Author

mzorko commented Feb 24, 2023

@juliasilge Hmm...I did some local tests yesterday and it took more than 1 hour to create 1,000+ pins in S3 with iris data (AWS slows down significantly after 200 API calls). Maybe it would make sense to do it once to create S3 pins example with more than 1,000 keys and to use that for testing. Will check current state of tests to see if we have any public bucket available for testing and to think how to implement this.

@juliasilge
Copy link
Member

Hmmmm, we do use a specific bucket for testing but our testing strategy now writes/reads and then tears it all down too; we don't have anything that persists across testing sessions. Let's say that writing a test for this will be impractical currently, and go for it as is.

Copy link
Member

@juliasilge juliasilge left a comment

Choose a reason for hiding this comment

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

Thank you again for this contribution @mzorko! 🚀

@juliasilge juliasilge merged commit a140321 into rstudio:main Feb 24, 2023
@github-actions
Copy link

This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants