Skip to content

HDFS-15869. Network issue while FSEditLogAsync is executing RpcEdit.logSyncNotify can cause the namenode to hang #2737

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
functioner wants to merge 9 commits into from
+36 −1
Closed

HDFS-15869. Network issue while FSEditLogAsync is executing RpcEdit.logSyncNotify can cause the namenode to hang #2737

Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
20be591
HDFS-15869. Network issue while FSEditLogAsync is executing RpcEdit.l…
functioner Mar 3, 2021
85a8948
Merge branch 'trunk' into HDFS-15869
functioner Mar 24, 2021
aaedc51
add multi-threaded executor
functioner Mar 24, 2021
bbc76cc
remove executor shutdown
functioner Apr 14, 2021
ee2e845
Merge branch 'trunk' into HDFS-15869
functioner Apr 15, 2021
6c856f3
add comments for logSyncNotifyExecutor
functioner Apr 15, 2021
3feb558
make logSyncNotifyExecutor size configurable
functioner Apr 15, 2021
b751835
add docs for the configuration
functioner Apr 15, 2021
3c57512
remove a useless blank line
functioner Apr 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion ...ject/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogAsync.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
import java.util.List;
import java.util.concurrent.ArrayBlockingQueue;
import java.util.concurrent.BlockingQueue;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Semaphore;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
Expand All @@ -48,6 +50,9 @@ class FSEditLogAsync extends FSEditLog implements Runnable {
// requires concurrent access from caller threads and syncing thread.
private final BlockingQueue<Edit> editPendingQ;

// Thread pool for executing logSyncNotify
private final ExecutorService logSyncNotifyExecutor;

// only accessed by syncing thread so no synchronization required.
// queue is unbounded because it's effectively limited by the size
// of the edit log buffer - ie. a sync will eventually be forced.
Expand All @@ -63,6 +68,9 @@ class FSEditLogAsync extends FSEditLog implements Runnable {
DFS_NAMENODE_EDITS_ASYNC_LOGGING_PENDING_QUEUE_SIZE_DEFAULT);

editPendingQ = new ArrayBlockingQueue<>(editPendingQSize);

// the thread pool size should be configurable later, and justified with a rationale
logSyncNotifyExecutor = Executors.newFixedThreadPool(10);
Copy link
Contributor

@linyiqun linyiqun Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to make this improvement be more configurable to use. Can we make thread pool size be configurable in this PR? if the pool size is configured as 0, that means this improvements is disabled.

Copy link
Contributor Author

@functioner functioner Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. What should be the default value? Many users use the default value, so probably we shouldn't set it as 0 by default.

Copy link
Contributor

@linyiqun linyiqun Mar 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@functioner , we could make 10 as the default pool size.

}

private boolean isSyncThreadAlive() {
Expand Down Expand Up @@ -117,6 +125,7 @@ void openForWrite(int layoutVersion) throws IOException {
public void close() {
super.close();
stopSyncThread();
logSyncNotifyExecutor.shutdown();
Copy link
Contributor

@Hexiaoqiao Hexiaoqiao Apr 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could meet issue when transition active to standby then transition back here. Because executor has been shutdown and no response will be sent. Moreover I suspect that namenode process could fatal in this case. In my opinion, we do not need to shutdown this executor. FYI. Thanks.

}

@Override
Expand Down Expand Up @@ -251,7 +260,9 @@ public void run() {
syncEx = ex;
}
while ((edit = syncWaitQ.poll()) != null) {
edit.logSyncNotify(syncEx);
final Edit notifyEdit = edit;
final RuntimeException ex = syncEx;
logSyncNotifyExecutor.submit(() -> notifyEdit.logSyncNotify(ex));
}
}
}
Expand Down