ctubbsii commented on a change in pull request #1602:
URL: https://github.com/apache/accumulo/pull/1602#discussion_r417005599
##########
File path:
server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManagerImpl.java
##########
@@ -353,7 +353,7 @@ public Path matchingFileSystem(Path source, Set<String>
options) {
URI optUri = URI.create(opt);
return sourceUri.getScheme().equals(optUri.getScheme())
&& Objects.equals(sourceUri.getAuthority(), optUri.getAuthority());
- }).map((String opt) -> new Path(opt)).findFirst().orElse(null);
+ }).map(Path::new).findFirst().orElse(null);
Review comment:
Path has strange and subtle differences between different constructors.
The original code specified the constructor was the one that took a String
explicitly. This code would work if the type was changed to URI, instead of
String, but I'm not sure the behavior would be identical. The original version
protects against subtle differences like that. For this reason, I'm not a big
fan of method references for constructors, unless the type is *very* stable and
well-known (like `ArrayList::new`).
The change you've made here is probably fine... but I'm not sure if we want
to protect against possible subtle changes in this code by keeping the explicit
String type declaration.
----------------------------------------------------------------
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]
