Skip to content

RATIS-2427. Close LogAppender for illegalStateException #1369

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

Merged
szetszwo merged 3 commits into from
Mar 12, 2026
+10 −4
Merged

RATIS-2427. Close LogAppender for illegalStateException #1369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c90c6a9
RATIS-2427. Close LogAppender for illegalStateException
symious Mar 9, 2026
5717fab
Revert "RATIS-2427. Close LogAppender for illegalStateException"
symious Mar 12, 2026
612cc2f
RATIS-2427. Check raftLog isOpen for restarting LogAppender
symious Mar 12, 2026
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
3 changes: 3 additions & 0 deletions ratis-server-api/src/main/java/org/apache/ratis/server/raftlog/RaftLog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ public interface RaftLog extends RaftLogSequentialOps, Closeable {
/** Invalid log index is used to indicate that the log index is missing. */
long INVALID_LOG_INDEX = LEAST_VALID_LOG_INDEX - 1;

/** Is this log already opened but not yet closed? */
boolean isOpened();

/** Does this log contains the given {@link TermIndex}? */
default boolean contains(TermIndex ti) {
Objects.requireNonNull(ti, "ti == null");
Expand Down
9 changes: 6 additions & 3 deletions ratis-server/src/main/java/org/apache/ratis/server/leader/LogAppenderBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,10 @@ public void start() {

@Override
public boolean isRunning() {
return daemon.isWorking() && server.getInfo().isLeader();
return daemon.isWorking()
&& server.getInfo().isAlive()
&& server.getInfo().isLeader()
&& getRaftLog().isOpened();
}
Comment on lines 126 to 131
Copy link
Contributor

@ivandika3 ivandika3 Mar 12, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Should we take into account the Thread.currentThread().isInterrupted()?

Saw the following InterruptedIOException from GrpcLogAppender#sleep which should set the current interrupt flag, but seems it's never checked

3e6753e7-cf24-4627-a99a-d02af1901b68@group-7B917680D70D->91193815-8c02-4e50-8cdc-792d6a5c1cdd-GrpcLogAppender-LogAppenderDaemon I/O was interrupted: java.io.InterruptedIOException: Interrupted appendLog, heartbeat? false

Copy link
Contributor

@szetszwo szetszwo Mar 12, 2026

Choose a reason for hiding this comment

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

Thread.currentThread().isInterrupted() is a runtime state but not an object state. So, it probably should not be in LogAppenderBase.isRunning().

Hypothetically, suppose we have a CLI to ask if a LogAppender is running. The CLI processing thread will call LogAppender.isRunning(). However, LogAppender.isRunning() has nothing to do with whether the CLI processing thread is interrupted.

Saw the following InterruptedIOException from GrpcLogAppender#sleep which should set the current interrupt flag, but seems it's never checked ...

The InterruptedIOException should be caught in LogAppenderDaemon.run(). Isn't it?

Copy link
Contributor

@ivandika3 ivandika3 Mar 12, 2026

Choose a reason for hiding this comment

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

Yes, you're correct, it should be checked in the daemon itself, not in API that can be used in other threads.


@Override
Expand All @@ -133,8 +136,8 @@ public CompletableFuture<LifeCycle.State> stopAsync() {
}

void restart() {
if (!server.getInfo().isAlive()) {
LOG.warn("Failed to restart {}: server {} is not alive", this, server.getMemberId());
if (!isRunning()) {
LOG.warn("{} is not running: skipping restart", this);
return;
}
getLeaderState().restart(this);
Expand Down
2 changes: 1 addition & 1 deletion ratis-server/src/main/java/org/apache/ratis/server/raftlog/RaftLogBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ public void checkLogState() {
state.assertOpen();
}

/** Is this log already opened? */
@Override
public boolean isOpened() {
return state.isOpened();
}
Expand Down
Loading