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

Reconcile chunk_limit and single_download_limit #332

Merged
merged 14 commits into from
Oct 5, 2023

Conversation

ateucher
Copy link
Collaborator

#330 surfaced the change to the single download limit on the WFS server to 500 records (down from 10,000). This exposed the potential incompatibility between bcdcd.single_download_limit and bcdc.chunk_limit options, where it was possible for the latter to be larger than the former. Since bcdcd.single_download_limit determined whether or not we needed pagination for a request, and bcdc.chunk_limit is used to determined the page size (number of records requested in a page), we were getting into the situation where we were attempting to retrieve more records in a page than is allowed by the server.

This PR sets the default chunk limit to the bcdc.single_download_limit by default, and also checks more comprehensively that the two values are compatible.

This address part of #330, but unfortunately does not fix all of it.

* Also have check_chunk_limit return the chunk limit to avoid repeated calls to bcdc_single_download_limit.
* Use check_chunk_limit in paginated requests
* Don't use a default
* Return chunk_limit early if chunk_value is NULL
@ateucher
Copy link
Collaborator Author

All passing now!

Copy link
Collaborator

@boshek boshek left a comment

Choose a reason for hiding this comment

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

I honestly can't remember why I had these separate other than to trigger pagination. thanks for doing this.

R/bcdc_options.R Outdated Show resolved Hide resolved
@ateucher
Copy link
Collaborator Author

Thanks both! I guess the remaining question is - should I remove one of the options (likely bcdata.single_download_limit, as that should always be retrieved from the server)? I think it would remove a lot of the gymnastics we're doing now, and I can't see the case where someone would want to set both manually.

@stephhazlitt
Copy link
Member

Is there ever a use case where a user would want to set a lower bcdata.single_download_limit? If no, then it makes sense to remove the option.

@ateucher
Copy link
Collaborator Author

Not really - as it can be done with the chunk limit.

@boshek
Copy link
Collaborator

boshek commented Sep 28, 2023

I think that if we do remove it, we should probably properly deprecated. I'd advocate just to leave it for now rather than risk breaking anyone's code.

* wrapper function to consult the option and warn once per session if it is set.
* Use check_chunk_limit throughout to be more efficient in checking both options
* Update documentation
* remove context()
* update expect_is() to expect_s3_class()
  and expect_type()
@ateucher
Copy link
Collaborator Author

ateucher commented Oct 3, 2023

Summary of new changes @boshek @stephhazlitt:

Copy link
Collaborator

@boshek boshek left a comment

Choose a reason for hiding this comment

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

Just a couple place where I think some comments would help.

R/bcdc_options.R Show resolved Hide resolved
R/utils-classes.R Outdated Show resolved Hide resolved
tests/testthat/test-options.R Show resolved Hide resolved
R/bcdc_options.R Outdated Show resolved Hide resolved
R/bcdc_options.R Outdated Show resolved Hide resolved
R/bcdc_options.R Outdated Show resolved Hide resolved
ateucher and others added 3 commits October 4, 2023 10:45
Co-authored-by: Stephanie Hazlitt <[email protected]>
Co-authored-by: Stephanie Hazlitt <[email protected]>
Co-authored-by: Stephanie Hazlitt <[email protected]>
@ateucher ateucher merged commit 64ebee4 into main Oct 5, 2023
7 checks passed
@ateucher ateucher deleted the fix-download-chunk-limit branch October 5, 2023 19:16
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.

3 participants