steveloughran commented on code in PR #5054:
URL: https://github.com/apache/hadoop/pull/5054#discussion_r1032391988
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/prefetch/S3APrefetchFakes.java:
##########
@@ -335,7 +341,8 @@ protected void writeFile(Path path, ByteBuffer buffer)
throws IOException {
private long fileCount = 0;
@Override
- protected Path getCacheFilePath() throws IOException {
+ protected Path getCacheFilePath(Configuration conf, LocalDirAllocator
localDirAllocator)
Review Comment:
think this should be indented more
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java:
##########
@@ -1561,7 +1561,7 @@ private FSDataInputStream executeOpen(
readContext.build(),
createObjectAttributes(path, fileStatus),
createInputStreamCallbacks(auditSpan),
- inputStreamStats));
+ inputStreamStats, getConf()));
Review Comment:
put on a new line for consistency
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3APrefetchingInputStream.java:
##########
@@ -60,8 +61,7 @@ public ITestS3APrefetchingInputStream() {
super(true);
}
- private static final Logger LOG =
- LoggerFactory.getLogger(ITestS3APrefetchingInputStream.class);
+ private static final Logger LOG =
LoggerFactory.getLogger(ITestS3APrefetchingInputStream.class);
Review Comment:
not needed; just revert to make patch smaller. unless you really, really
want this
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/impl/prefetch/CachingBlockManager.java:
##########
@@ -129,6 +138,8 @@ public CachingBlockManager(
this.ops = new BlockOperations();
this.ops.setDebug(false);
+ this.conf = conf;
Review Comment:
add requireNonNull() checks here
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3APrefetchingInputStream.java:
##########
@@ -294,4 +294,34 @@ public void testStatusProbesAfterClosingStream() throws
Throwable {
}
+ @Test
+ public void testCacheFileExistence() throws Throwable {
Review Comment:
is there any cleanup of these files after? i know a maven clean will do it,
but these may be be big files and create problems on docker container runs.
best to do an rm of the dir, or at least the files, afterwards
--
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]
