You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by "RkGrit (via GitHub)" <gi...@apache.org> on 2025/04/01 14:11:09 UTC
[PR] RATIS-2271 Leadership Loss Causes ClosedByInterruptException and NullPointerException in LogAppender Thread [ratis]
RkGrit opened a new pull request, #1245:
URL: https://github.com/apache/ratis/pull/1245
## What changes were proposed in this pull request?
After a leader loses leadership due to heartbeat timeout with a majority of followers, it forcibly interrupts the GrpcLogAppender thread.
This abrupt termination leads to two critical exceptions during log file reads:
1. ClosedByInterruptException when initializing SegmentedRaftLogInputStream.
2. A cascading NullPointerException in LogSegment.loadCache() due to incomplete log loading.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-2271?page=com.atlassian.jira.plugin.system.issuetabpanels%3Aall-tabpanel
## How was this patch tested?
unit tests
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2271 Leadership Loss Causes ClosedByInterruptException and NullPointerException in LogAppender Thread [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1245:
URL: https://github.com/apache/ratis/pull/1245#discussion_r2025452956
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogReader.java:
##########
@@ -169,23 +163,28 @@ public long skip(long amt) throws IOException {
*/
boolean verifyHeader() throws IOException {
final int headerLength = SegmentedRaftLogFormat.getHeaderLength();
- final int readLength = in.read(temp, 0, headerLength);
- Preconditions.assertTrue(readLength <= headerLength);
- final int matchLength = SegmentedRaftLogFormat.matchHeader(temp, 0, readLength);
- Preconditions.assertTrue(matchLength <= readLength);
-
- if (readLength == headerLength && matchLength == readLength) {
- // The header is matched successfully
- return true;
- } else if (SegmentedRaftLogFormat.isTerminator(temp, matchLength, readLength - matchLength)) {
- // The header is partially written
- return false;
+ try{
Review Comment:
Let's try-catch only the read, i.e.
```diff
@@ -169,7 +163,14 @@ class SegmentedRaftLogReader implements Closeable {
*/
boolean verifyHeader() throws IOException {
final int headerLength = SegmentedRaftLogFormat.getHeaderLength();
- final int readLength = in.read(temp, 0, headerLength);
+ final int readLength;
+ try {
+ readLength = in.read(temp, 0, headerLength);
+ } catch (ClosedByInterruptException e) {
+ Thread.currentThread().interrupt();
+ throw new IOException("Interrupted while reading the header of " + file, e);
+ }
+
```
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogReader.java:
##########
@@ -34,14 +34,8 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-import java.io.BufferedInputStream;
-import java.io.Closeable;
-import java.io.DataInputStream;
-import java.io.EOFException;
-import java.io.File;
-import java.io.FilterInputStream;
-import java.io.IOException;
-import java.io.InputStream;
+import java.io.*;
Review Comment:
Please don't change the existing imports.
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogInputStream.java:
##########
@@ -103,8 +104,12 @@ public LogEntryProto nextEntry() throws IOException {
if (state.isUnopened()) {
try {
init();
- } catch (Exception e) {
Review Comment:
Let's keep catching `Exception`. It will print an error for other exceptions.
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogInputStream.java:
##########
@@ -103,8 +104,12 @@ public LogEntryProto nextEntry() throws IOException {
if (state.isUnopened()) {
try {
init();
- } catch (Exception e) {
- LOG.error("caught exception initializing " + this, e);
+ } catch (IOException e) {
+ if (e.getCause() instanceof ClosedByInterruptException) {
+ LOG.warn("Initialization is interrupted: {}", this, e);
+ } else {
+ LOG.error("caught exception initializing " + this, e);
Review Comment:
Could you also update the log message?
```java
LOG.error("Failed to initialize {}", this, e);
```
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/LogSegment.java:
##########
@@ -292,7 +292,7 @@ ReferenceCountedObject<LogEntryProto> load(TermIndex key) throws IOException {
}
});
loadingTimes.incrementAndGet();
- return Objects.requireNonNull(toReturn.get(), () -> "toReturn == null for " + key);
+ return toReturn.get();
Review Comment:
We should throw `RaftLogIOException` if it is null.
```java
+++ b/ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/LogSegment.java
@@ -292,7 +292,11 @@ public final class LogSegment {
}
});
loadingTimes.incrementAndGet();
- return Objects.requireNonNull(toReturn.get(), () -> "toReturn == null for " + key);
+ final ReferenceCountedObject<LogEntryProto> proto = toReturn.get();
+ if (proto == null) {
+ throw new RaftLogIOException("Failed to load log entry " + key);
+ }
+ return proto;
}
}
@@ -502,8 +506,10 @@ public final class LogSegment {
}
try {
return cacheLoader.load(ti);
+ } catch (RaftLogIOException e) {
+ throw e;
} catch (Exception e) {
- throw new RaftLogIOException(e);
+ throw new RaftLogIOException("Failed to loadCache for log entry " + ti, e);
}
}
```
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2271 Leadership Loss Causes ClosedByInterruptException and NullPointerException in LogAppender Thread [ratis]
Posted by "RkGrit (via GitHub)" <gi...@apache.org>.
RkGrit commented on code in PR #1245:
URL: https://github.com/apache/ratis/pull/1245#discussion_r2028174985
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogInputStream.java:
##########
@@ -103,8 +104,12 @@ public LogEntryProto nextEntry() throws IOException {
if (state.isUnopened()) {
try {
init();
- } catch (Exception e) {
- LOG.error("caught exception initializing " + this, e);
+ } catch (IOException e) {
+ if (e.getCause() instanceof ClosedByInterruptException) {
+ LOG.warn("Initialization is interrupted: {}", this, e);
+ } else {
+ LOG.error("caught exception initializing " + this, e);
Review Comment:
Sure, no problem.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2271 Leadership Loss Causes ClosedByInterruptException and NullPointerException in LogAppender Thread [ratis]
Posted by "RkGrit (via GitHub)" <gi...@apache.org>.
RkGrit commented on code in PR #1245:
URL: https://github.com/apache/ratis/pull/1245#discussion_r2028164269
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogReader.java:
##########
@@ -169,23 +163,28 @@ public long skip(long amt) throws IOException {
*/
boolean verifyHeader() throws IOException {
final int headerLength = SegmentedRaftLogFormat.getHeaderLength();
- final int readLength = in.read(temp, 0, headerLength);
- Preconditions.assertTrue(readLength <= headerLength);
- final int matchLength = SegmentedRaftLogFormat.matchHeader(temp, 0, readLength);
- Preconditions.assertTrue(matchLength <= readLength);
-
- if (readLength == headerLength && matchLength == readLength) {
- // The header is matched successfully
- return true;
- } else if (SegmentedRaftLogFormat.isTerminator(temp, matchLength, readLength - matchLength)) {
- // The header is partially written
- return false;
+ try{
Review Comment:
Thanks very much for the patch! You are correct that we should minimize the scope of try-catch.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2271 Leadership Loss Causes ClosedByInterruptException and NullPointerException in LogAppender Thread [ratis]
Posted by "RkGrit (via GitHub)" <gi...@apache.org>.
RkGrit commented on PR #1245:
URL: https://github.com/apache/ratis/pull/1245#issuecomment-2777706759
@szetszwo, Thank you very much for suggesting these changes. I have applied these suggestions and look forward to your reply.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2271 Leadership Loss Causes ClosedByInterruptException and NullPointerException in LogAppender Thread [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo merged PR #1245:
URL: https://github.com/apache/ratis/pull/1245
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2271 Leadership Loss Causes ClosedByInterruptException and NullPointerException in LogAppender Thread [ratis]
Posted by "RkGrit (via GitHub)" <gi...@apache.org>.
RkGrit commented on PR #1245:
URL: https://github.com/apache/ratis/pull/1245#issuecomment-2769510542
@szetszwo
Please review this PR
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2271 Leadership Loss Causes ClosedByInterruptException and NullPointerException in LogAppender Thread [ratis]
Posted by "RkGrit (via GitHub)" <gi...@apache.org>.
RkGrit commented on code in PR #1245:
URL: https://github.com/apache/ratis/pull/1245#discussion_r2028183190
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/LogSegment.java:
##########
@@ -292,7 +292,7 @@ ReferenceCountedObject<LogEntryProto> load(TermIndex key) throws IOException {
}
});
loadingTimes.incrementAndGet();
- return Objects.requireNonNull(toReturn.get(), () -> "toReturn == null for " + key);
+ return toReturn.get();
Review Comment:
Yes, it seems more safe.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2271 Leadership Loss Causes ClosedByInterruptException and NullPointerException in LogAppender Thread [ratis]
Posted by "RkGrit (via GitHub)" <gi...@apache.org>.
RkGrit commented on code in PR #1245:
URL: https://github.com/apache/ratis/pull/1245#discussion_r2028185308
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogReader.java:
##########
@@ -34,14 +34,8 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-import java.io.BufferedInputStream;
-import java.io.Closeable;
-import java.io.DataInputStream;
-import java.io.EOFException;
-import java.io.File;
-import java.io.FilterInputStream;
-import java.io.IOException;
-import java.io.InputStream;
+import java.io.*;
Review Comment:
Got it.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2271 Leadership Loss Causes ClosedByInterruptException and NullPointerException in LogAppender Thread [ratis]
Posted by "RkGrit (via GitHub)" <gi...@apache.org>.
RkGrit commented on code in PR #1245:
URL: https://github.com/apache/ratis/pull/1245#discussion_r2028174130
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogInputStream.java:
##########
@@ -103,8 +104,12 @@ public LogEntryProto nextEntry() throws IOException {
if (state.isUnopened()) {
try {
init();
- } catch (Exception e) {
Review Comment:
Good suggestion! I will modify this.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org