danielcweeks commented on pull request #1573: URL: https://github.com/apache/iceberg/pull/1573#issuecomment-716742112
> @danielcweeks, I did a fairly thorough review and found some problems in the read path (not updating positions, seek falling back to new connection). Those also expose some test gaps. > > Otherwise, there are two things I'd fix: > > * It would be really nice to depend on only one version of the AWS SDK. If the URI class is only used to parse path and bucket, can we replace it with something? @jackye1995, any ideas? > * Implement `create`. We can do this in a follow-up, but it seems like it will bite us if we don't and forget I expanded the read path testing to cover a number of other cases. I agree it's not ideal to depend on v1 and v2 of the SDK, but they are intended to coexist and from what I can see there isn't a current alternative to the `AmazonS3URI` class, I'd rather not create a utility just to mimic that behavior. I actually disagree with implementing `create()` because, as I mentioned earlier, just about any scenario depending on "create if not exists" behavior is risky with S3. Considering that the object doesn't even appear until a stream is closed, which can be any amount of time, it leads to any number of potential race conditions. Seeing as how `create()` is not used anywhere currently, we should leave this unimplemented until the need arises and revisit this decision at that time. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
