ctubbsii commented on a change in pull request #369: [ACCUMULO-4787] Close 
input stream in AccumuloReplicaSystem
URL: https://github.com/apache/accumulo/pull/369#discussion_r167682325
 
 

 ##########
 File path: 
server/tserver/src/main/java/org/apache/accumulo/tserver/log/LogSorter.java
 ##########
 @@ -113,44 +113,46 @@ public void sort(String name, Path srcPath, String 
destPath) {
         // the following call does not throw an exception if the file/dir does 
not exist
         fs.deleteRecursively(new Path(destPath));
 
-        DFSLoggerInputStreams inputStreams;
-        try {
-          inputStreams = DfsLogger.readHeaderAndReturnStream(fs, srcPath, 
conf);
-        } catch (LogHeaderIncompleteException e) {
-          log.warn("Could not read header from write-ahead log " + srcPath + 
". Not sorting.");
-          // Creating a 'finished' marker will cause recovery to proceed 
normally and the
-          // empty file will be correctly ignored downstream.
-          fs.mkdirs(new Path(destPath));
-          writeBuffer(destPath, Collections.<Pair<LogFileKey,LogFileValue>> 
emptyList(), part++);
-          fs.create(SortedLogState.getFinishedMarkerPath(destPath)).close();
-          return;
-        }
+        try (final FSDataInputStream fsinput = fs.open(srcPath)) {
+          DFSLoggerInputStreams inputStreams;
+          try {
+            inputStreams = DfsLogger.readHeaderAndReturnStream(fsinput, conf);
 
 Review comment:
   I'm not exactly sure what kind of additional resources the crypto stream 
might use, but it seems to me that since `inputStreams` is not `Closeable`, it 
may be possible for an the `fsinput` to be closed, but the crypto stream in 
`inputStreams` to still be holding resources. Would need to dig further to be 
sure, but it would be nice if `DFSLoggerInputStreams` was `Closeable` and this 
was created as a second resource, in the `try-with-resources` block on line 116.
   
   Same comment applies to other uses of `DFSLoggerInputStreams`.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Reply via email to