morningman commented on code in PR #69525:
URL: https://github.com/apache/doris/pull/69525#discussion_r4359746323
##########
fe/fe-core/src/main/java/org/apache/doris/persist/EditLog.java:
##########
@@ -265,8 +282,7 @@ private void flushEditLog() {
if (journal instanceof BDBJEJournal) {
LOG.error("BDBJE stats : {}", ((BDBJEJournal)
journal).getBDBStats());
}
- LOG.error("Fatal Error : write stream Exception", t);
- System.exit(-1);
+ exitOnWriteFailure(t);
Review Comment:
Fixed in a2ea9eb50f0.
Every exit that `BDBJEJournal` takes on its own on the write path now goes
through `EditLog.exitOnWriteFailure` (made public for this), which sets the
flag before `System.exit(-1)`. That is `ReplicaWriteException` and the
exhausted `DatabaseException` retries in `write(JournalBatch)`, the same two in
`write(short, Writable)`, and one more that this comment did not list:
`rollJournal` finding a journal count it did not expect. The roll happens on
the flusher thread after each batch and on the direct-write path, so it is a
write failure as well. The existing log and stderr lines of these exits are
unchanged. The other `System.exit` calls in `BDBJEJournal` and `BDBEnvironment`
are on open, recovery and replay paths, not on the write path of the master.
I did not propagate the failures to the `EditLog` handler instead, for two
reasons:
- `rollEditLog()` is called after the try/catch of `flushEditLog()`, and the
flusher loop has no catch. An exception from `rollJournal` would end the
`EditLog-Flusher` thread without exiting, and every writer would wait forever.
- `logEdit(short, List)` also calls `journal.write(batch)`. Its caller,
`CloudTabletRebalancer`, catches the exception, logs it and goes on, so a write
failure there would no longer stop the frontend.
Test: `BDBJEJournalTest#testWriteFailuresExitThroughEditLog` runs two real
failures of the journal against a mocked `Database`: a direct write whose put
never succeeds, and a roll whose database holds more journals than the ids
handed out. Only `EditLog.exitOnWriteFailure` is stubbed (to throw), and the
test checks that each failure ends there. A unit test can not let the real
`System.exit` run, since that ends the test JVM, so the flag itself and the
hook are covered by `DorisFETest`. The test does not use the other two
branches: `ReplicaWriteException` only has a constructor that takes JE
internals, and the `DatabaseException` branch sleeps 5 s per retry.
##########
fe/fe-core/src/main/java/org/apache/doris/DorisFE.java:
##########
@@ -725,7 +727,15 @@ public static boolean isServerReady() {
return serverReady.get();
}
- private static void gracefulShutdown() {
+ @VisibleForTesting
+ static void gracefulShutdown() {
+ if (EditLog.isWriteFailed()) {
Review Comment:
Fixed in a2ea9eb50f0.
`gracefulShutdown` now checks `EditLog.isWriteFailed()` in the condition of
its wait loop instead of only on entry, so a write failure that happens while
the hook waits ends the wait at the next check, within a second. The loop
already looks at the running queries once per second, so I did not add a
separate wake-up. Together with the fix for the BDB exits, this also covers a
master that is demoted: the exit of the state listener starts the hook, and the
commit that then fails in `BDBJEJournal` sets the flag.
Test:
`DorisFETest#testGracefulShutdownStopsWaitingWhenEditLogWriteFailsWhileWaiting`
registers a query, starts the hook in a thread, waits until the thread sleeps
in the loop, then sets the flag and expects the thread to end within 10 s.
Against the previous revision of this PR, where the flag was checked only on
entry, the test finds the hook still waiting after 10 s.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
