-
Notifications
You must be signed in to change notification settings - Fork 24
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
Distinguish between Error and Empty for GCS and S3 DataVaults #7198
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation looks good to me :)
Props for writing tests 🎉
The test does not currently distinguish between Failure and Empty, though. Also, could you add a test case where a Failure is expected? (e.g. Forbidden?)
I think this is actually a user-facing change, I’d add something like this into the changelog
“Correctly use configured fill-value for missing chunks of remote datasets hosted on gcs or s3.” (since there is a distinction on Failure vs Empty in ChunkReader, which can now work here)
For S3 I would need a public data set to actually validate that it works in the test. I haven't found anything by Amazon, so the tests could use some public dataset known to us or just test failure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s fair then, I don’t have such a link ready either.
LGTM!
@fm3 Tested it again before merging and a dataset with required credentials did not work because of "timeout waiting for connection pool". Turns out one has to close an s3object. |
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
Since it is not user facing I guess no changelog.
(Please delete unneeded items, merge only when none are left open)