phet commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1097909721
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -523,11 +509,14 @@ public void cleanup() {
}
}
- protected static class TestLocalFileSystem extends LocalFileSystem {
-
+ protected class TestLocalFileSystem extends LocalFileSystem {
+ ConcurrentHashMap<Path, List<AclEntry>> pathToAclEntries = new
ConcurrentHashMap<>();
@Override
public void setAcl(Path path, List<AclEntry> aclEntries) {
pathToAclEntries.put(path, aclEntries);
}
+ public ConcurrentHashMap<Path, List<AclEntry>> getPathToAclEntries() {
+ return pathToAclEntries;
Review Comment:
first off, I recognize this is just a unit test... but still I'd look for it
to reach at least somewhere on this spectrum:
1. the return type shouldn't be `ConcurrentHashMap`--because the caller's
not supposed to write to it!
2. return `ImmutableMap` so the caller breaks on any misguided attempt to
write to it (even though not isolated from concurrent puts)
3. best practice of a defensive copy by returning a new map (to freeze it at
a point in time). here, should be `synchronized` to avoid concurrent
modification exception during iterateration
--
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]
