[GitHub] [accumulo] EdColeman commented on a change in pull request #2117: Change sorted recovery to write to RFiles

[GitBox <gi...@apache.org>]

EdColeman commented on a change in pull request #2117:
URL: https://github.com/apache/accumulo/pull/2117#discussion_r640065308



##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/log/RecoveryLogsIterator.java
##########
@@ -93,11 +107,48 @@ public void remove() {
 
   @Override
   public void close() {
-    for (CloseableIterator<?> reader : iterators) {
-      try {
-        reader.close();
-      } catch (IOException e) {
-        LOG.debug("Failed to close reader", e);
+    scanners.forEach(ScannerBase::close);
+  }
+
+  /**
+   * Check for sorting signal files (finished/failed) and get the logs in the provided directory.
+   */
+  private List<Path> getFiles(VolumeManager fs, Path directory) throws IOException {
+    boolean foundFinish = false;
+    List<Path> logFiles = new ArrayList<>();
+    for (FileStatus child : fs.listStatus(directory)) {
+      if (child.getPath().getName().startsWith("_"))
+        continue;
+      if (SortedLogState.isFinished(child.getPath().getName())) {
+        foundFinish = true;
+        continue;
+      }
+      if (SortedLogState.FAILED.getMarker().equals(child.getPath().getName())) {
+        continue;
+      }
+      FileSystem ns = fs.getFileSystemByPath(child.getPath());

Review comment:
       I am not sure if this applies here, or if it is still an issue with hadoop  (maybe behavior has changed)  But if this close is acting on a hadoop file system, you need to check that you are not closing the underlying hadoop file system for all clients. (see [stack-overflow](https://stackoverflow.com/questions/20057881/hadoop-filesystem-closed-exception-when-doing-bufferedreader-close) ). 
   
   `There is a little-known gotcha with the hadoop filesystem API: FileSystem.get returns the same object for every invocation with the same filesystem. So if one is closed anywhere, they are all closed. You could debate the merits of this decision, but that's the way it is.`
   
    Its going to depend on if the fs.get() is returning a shared hdfs resource or a unique one to say if a close is appropriate here and in the other case mentioned.




-- 
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:
users@infra.apache.org