steveloughran commented on code in PR #6167:
URL: https://github.com/apache/hadoop/pull/6167#discussion_r1372128380
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java:
##########
@@ -1652,7 +1653,12 @@ public Path makeQualified(final Path path) {
LOG.debug("Stripping trailing '/' from {}", q);
// deal with an empty "/" at the end by mapping to the parent and
// creating a new path from it
- q = new Path(urlString.substring(0, urlString.length() - 1));
+ try {
+ q = new Path(new URI(urlString.substring(0, urlString.length() -
1)));
+ } catch (URISyntaxException e) {
+ LOG.error(String.format("Error removing trailing / from path %s",
path.toString()));
Review Comment:
1. We'd want a LogExactlyOnce so if there is a problem it only ever gets
logged once, not every time a path was loaded.
2. it should be at warn, not error
3. +use slf4j {} expansions.
thanks
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java:
##########
@@ -1652,7 +1653,12 @@ public Path makeQualified(final Path path) {
LOG.debug("Stripping trailing '/' from {}", q);
Review Comment:
how about pulling out everything in this method after the
super.makeQualified(path); call into a static method in S3AUtils? that way it
could be tested in a unit test rather than waiting until someone runs the
integration tests. faster and with yetus coverage
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
