Skip to content

HBASE-28850 Only return from ReplicationSink.replicationEntries while… #6263

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
merged 1 commit into from
Sep 19, 2024
+19 −2
Merged

HBASE-28850 Only return from ReplicationSink.replicationEntries while… #6263

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 19 additions & 2 deletions ...erver/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,16 +492,33 @@ private void batch(TableName tableName, Collection<List<Row>> allRows, int batch
}
futures.addAll(batchRows.stream().map(table::batchAll).collect(Collectors.toList()));
}
// Here we will always wait until all futures are finished, even if there are failures when
// getting from a future in the middle. This is because this method may be called in a rpc call,
// so the batch operations may reference some off heap cells(through CellScanner). If we return
// earlier here, the rpc call may be finished and they will release the off heap cells before
// some of the batch operations finish, and then cause corrupt data or even crash the region
// server. See HBASE-28584 and HBASE-28850 for more details.
IOException error = null;
for (Future<?> future : futures) {
try {
FutureUtils.get(future);
} catch (RetriesExhaustedException e) {
IOException ioe;
if (e.getCause() instanceof TableNotFoundException) {
throw new TableNotFoundException("'" + tableName + "'");
ioe = new TableNotFoundException("'" + tableName + "'");
} else {
ioe = e;
}
if (error == null) {
error = ioe;
} else {
error.addSuppressed(ioe);
Copy link
Contributor

@virajjasani virajjasani Sep 18, 2024

Choose a reason for hiding this comment

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

Logging the exception before it is suppressed might be helpful? (unless suppressed exceptions are all logged with original exception, which i don't recall if it happens)

Copy link
Contributor Author

@Apache9 Apache9 Sep 19, 2024

Choose a reason for hiding this comment

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

  public static void main(String[] args) {
    Exception error = new Exception("root");
    error.addSuppressed(new IOException("suppressed1"));
    error.addSuppressed(new IOException("suppressed2"));
    error.printStackTrace();
  }

The output

java.lang.Exception: root
	at org.apache.hadoop.hbase.regionserver.regionreplication.RegionReplicationSink.main(RegionReplicationSink.java:461)
	Suppressed: java.io.IOException: suppressed1
		at org.apache.hadoop.hbase.regionserver.regionreplication.RegionReplicationSink.main(RegionReplicationSink.java:462)
	Suppressed: java.io.IOException: suppressed2
		at org.apache.hadoop.hbase.regionserver.regionreplication.RegionReplicationSink.main(RegionReplicationSink.java:463)

So I think it is fine to not log it here?

}
throw e;
}
}
if (error != null) {
throw error;
}
}

private AsyncClusterConnection getConnection() throws IOException {
Expand Down