From 7c701f4144e8befc0c8a1e4f9a6f52b614ed78e2 Mon Sep 17 00:00:00 2001 From: Rushabh Shah Date: Fri, 3 Nov 2023 11:51:47 -0700 Subject: [PATCH 1/4] HBASE-28184 Tailing the WAL is very slow if there are multiple peers --- .../hbase/replication/regionserver/WALEntryStream.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/WALEntryStream.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/WALEntryStream.java index d1f85774a635..55c6796b8965 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/WALEntryStream.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/WALEntryStream.java @@ -470,7 +470,12 @@ private Pair readNextEntryAndRecordReaderPositi // in the future, if we want to optimize the logic here, for example, do not call this method // every time, or do not acquire rollWriteLock in the implementation of this method, we need to // carefully review the optimized implementation - OptionalLong fileLength = walFileLengthProvider.getLogFileSizeIfBeingWritten(currentPath); + OptionalLong fileLength = OptionalLong.empty(); + if (logQueue.getQueueSize(walGroupId) == 1) { + // Get the size of log file only if there is 1 log file in the queue. If there are + // more than 1 log file in the queue, then the head will never be the current WAL file. + walFileLengthProvider.getLogFileSizeIfBeingWritten(currentPath); + } WALTailingReader.Result readResult = reader.next(fileLength.orElse(-1)); long readerPos = readResult.getEntryEndPos(); Entry readEntry = readResult.getEntry(); From db7f754f84a04cf041b09b09d8efe9e67fe33755 Mon Sep 17 00:00:00 2001 From: Rushabh Shah Date: Fri, 3 Nov 2023 13:34:46 -0700 Subject: [PATCH 2/4] HBASE-28184 Tailing the WAL is very slow if there are multiple peers --- .../hadoop/hbase/replication/regionserver/WALEntryStream.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/WALEntryStream.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/WALEntryStream.java index 55c6796b8965..d59c210d769a 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/WALEntryStream.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/WALEntryStream.java @@ -474,7 +474,7 @@ private Pair readNextEntryAndRecordReaderPositi if (logQueue.getQueueSize(walGroupId) == 1) { // Get the size of log file only if there is 1 log file in the queue. If there are // more than 1 log file in the queue, then the head will never be the current WAL file. - walFileLengthProvider.getLogFileSizeIfBeingWritten(currentPath); + fileLength = walFileLengthProvider.getLogFileSizeIfBeingWritten(currentPath); } WALTailingReader.Result readResult = reader.next(fileLength.orElse(-1)); long readerPos = readResult.getEntryEndPos(); From ad005069bb0bb7dc67d659b05668ae4c3aef0de5 Mon Sep 17 00:00:00 2001 From: Rushabh Shah Date: Sat, 4 Nov 2023 09:26:40 -0700 Subject: [PATCH 3/4] HBASE-28184 Review comments --- .../regionserver/WALEntryStream.java | 23 +++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/WALEntryStream.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/WALEntryStream.java index d59c210d769a..852aae0ecbe8 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/WALEntryStream.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/WALEntryStream.java @@ -470,10 +470,25 @@ private Pair readNextEntryAndRecordReaderPositi // in the future, if we want to optimize the logic here, for example, do not call this method // every time, or do not acquire rollWriteLock in the implementation of this method, we need to // carefully review the optimized implementation - OptionalLong fileLength = OptionalLong.empty(); - if (logQueue.getQueueSize(walGroupId) == 1) { - // Get the size of log file only if there is 1 log file in the queue. If there are - // more than 1 log file in the queue, then the head will never be the current WAL file. + OptionalLong fileLength; + if (logQueue.getQueueSize(walGroupId) > 1) { + // if there are more than one files in queue, although it is possible that we are + // still trying to write the trailer of the file and it is not closed yet, we can + // make sure that we will not write any WAL entries to it any more, so it is safe + // to just let the upper layer try to read the whole file without limit + fileLength = OptionalLong.empty(); + } else { + // if there is only one file in queue, check whether it is still being written to + // we must call this before actually reading from the reader, as this method will acquire the + // rollWriteLock. This is very important, as we will enqueue the new WAL file in postLogRoll, + // and before this happens, we could have already finished closing the previous WAL file. If + // we do not acquire the rollWriteLock and return whether the current file is being written + // to, we may finish reading the previous WAL file and start to read the next one, before it + // is enqueued into the logQueue, thus lead to an empty logQueue and make the shipper think + // the queue is already ended and quit. See HBASE-28114 and related issues for more details. + // in the future, if we want to optimize the logic here, for example, do not call this method + // every time, or do not acquire rollWriteLock in the implementation of this method, we need + // to carefully review the optimized implementation fileLength = walFileLengthProvider.getLogFileSizeIfBeingWritten(currentPath); } WALTailingReader.Result readResult = reader.next(fileLength.orElse(-1)); From 6f2ef86bf6ed1a176f6372e7562b0e888bcdd798 Mon Sep 17 00:00:00 2001 From: Rushabh Shah Date: Mon, 6 Nov 2023 11:50:57 -0800 Subject: [PATCH 4/4] Addressing review comments --- .../hbase/replication/regionserver/WALEntryStream.java | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/WALEntryStream.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/WALEntryStream.java index 852aae0ecbe8..186d5b7c4d18 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/WALEntryStream.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/WALEntryStream.java @@ -460,16 +460,6 @@ private void dequeueCurrentLog() { * Returns whether the file is opened for writing. */ private Pair readNextEntryAndRecordReaderPosition() { - // we must call this before actually reading from the reader, as this method will acquire the - // rollWriteLock. This is very important, as we will enqueue the new WAL file in postLogRoll, - // and before this happens, we could have already finished closing the previous WAL file. If we - // do not acquire the rollWriteLock and return whether the current file is being written to, we - // may finish reading the previous WAL file and start to read the next one, before it is - // enqueued into the logQueue, thus lead to an empty logQueue and make the shipper think the - // queue is already ended and quit. See HBASE-28114 and related issues for more details. - // in the future, if we want to optimize the logic here, for example, do not call this method - // every time, or do not acquire rollWriteLock in the implementation of this method, we need to - // carefully review the optimized implementation OptionalLong fileLength; if (logQueue.getQueueSize(walGroupId) > 1) { // if there are more than one files in queue, although it is possible that we are