[GitHub] [hbase] apurtell commented on a change in pull request #3636: HBASE-26106 AbstractFSWALProvider#getArchivedLogPath doesn't look for wal file in all oldWALs directory.

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

apurtell commented on a change in pull request #3636:
URL: https://github.com/apache/hbase/pull/3636#discussion_r699501624



##########
File path: hbase-mapreduce/src/test/java/org/apache/hadoop/hbase/mapreduce/TestWALRecordReader.java
##########
@@ -335,13 +342,16 @@ private void testSplitWithMovingWAL(InputSplit split, byte[] col1, byte[] col2)
     // Move log file to archive directory
     // While WAL record reader is open
     WALInputFormat.WALSplit split_ = (WALInputFormat.WALSplit) split;
-
     Path logFile = new Path(split_.getLogFileName());
-    Path archivedLog = AbstractFSWALProvider.getArchivedLogPath(logFile, conf);
-    boolean result = fs.rename(logFile, archivedLog);
-    assertTrue(result);
-    result = fs.exists(archivedLog);
-    assertTrue(result);
+    Path archivedLogDir = getWALArchiveDir(conf);
+    Path archivedLogLocation = new Path(archivedLogDir, logFile.getName());
+    assertNotEquals(split_.getLogFileName(), archivedLogLocation.toString());
+
+    assertTrue(fs.rename(logFile, archivedLogLocation));
+    assertTrue(fs.exists(archivedLogDir));
+    assertFalse(fs.exists(logFile));
+    // TODO: This is not behaving as expected. WALInputFormat#WALKeyRecordReader doesn't open

Review comment:
       I was going to cut 2.4.6RC0 today but saw this issue. Will wait until this is merged to branch-2.4. 

##########
File path: hbase-mapreduce/src/main/java/org/apache/hadoop/hbase/mapreduce/WALInputFormat.java
##########
@@ -217,8 +217,9 @@ public boolean nextKeyValue() throws IOException, InterruptedException {
         }
         return res;
       } catch (IOException e) {
-        Path archivedLog = AbstractFSWALProvider.getArchivedLogPath(logFile, conf);
-        if (logFile != archivedLog) {
+        Path archivedLog = AbstractFSWALProvider.findArchivedLog(logFile, conf);
+        // archivedLog can be null if unable to locate in archiveDir.
+        if (archivedLog != null && logFile != archivedLog) {

Review comment:
       Should this be `!(logFile.equals(archivedLog)) `or is object equality good enough here? As these are paths there might be more than one object instance (so different regards object equality) but representing equivalent strings?

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/wal/AbstractFSWALProvider.java
##########
@@ -557,8 +527,9 @@ public static Path findArchivedLog(Path path, Configuration conf) throws IOExcep
         return reader;
       } catch (FileNotFoundException fnfe) {
         // If the log was archived, continue reading from there
-        Path archivedLog = AbstractFSWALProvider.getArchivedLogPath(path, conf);
-        if (!Objects.equals(path, archivedLog)) {
+        Path archivedLog = AbstractFSWALProvider.findArchivedLog(path, conf);
+        // archivedLog can be null if unable to locate in archiveDir.
+        if (archivedLog != null && !Objects.equals(path, archivedLog)) {

Review comment:
       Yeah see my above comment. Here you use Objects.equals which will call through to Path's equals method. This is along the lines of what I proposed, I think you should do this in the other place too.




-- 
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: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org