Skip to content

HBASE-28175: Deep copy RpcLogDetails' param field #5481

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
bbeaudreault merged 4 commits into from
Nov 9, 2023
Merged

HBASE-28175: Deep copy RpcLogDetails' param field #5481

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eafcd3f
Deep copy RpcLogDetails' param field
Oct 26, 2023
814d5df
simplify constructor, test byte buffer corruption
Nov 2, 2023
41b36db
add classrule
Nov 2, 2023
c60240a
simplify RpcLogDetails constructor catch
Nov 6, 2023
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
2 changes: 1 addition & 1 deletion hbase-client/src/main/java/org/apache/hadoop/hbase/client/SlowLogParams.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ public boolean equals(Object o) {
}
SlowLogParams that = (SlowLogParams) o;
return new EqualsBuilder().append(regionName, that.regionName).append(params, that.params)
.append("scan", scan).isEquals();
.append(scan, that.scan).isEquals();
Copy link
Contributor Author

@rmdmattingly rmdmattingly Oct 26, 2023

Choose a reason for hiding this comment

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

I also noticed this bug in the SlowLogParams equals implementation, but I don't think it's related to the slow log payload corruption that we've observed

}

@Override
Expand Down
44 changes: 42 additions & 2 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/namequeues/RpcLogDetails.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,14 @@
import org.apache.commons.lang3.builder.ToStringBuilder;
import org.apache.hadoop.hbase.ipc.RpcCall;
import org.apache.yetus.audience.InterfaceAudience;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import org.apache.hbase.thirdparty.com.google.protobuf.InvalidProtocolBufferException;
import org.apache.hbase.thirdparty.com.google.protobuf.Message;

import org.apache.hadoop.hbase.shaded.protobuf.generated.ClientProtos;

/**
* RpcCall details that would be passed on to ring buffer of slow log responses
*/
Expand All @@ -32,8 +37,10 @@ public class RpcLogDetails extends NamedQueuePayload {

public static final int SLOW_LOG_EVENT = 0;

private static final Logger LOG = LoggerFactory.getLogger(RpcLogDetails.class.getName());

private final RpcCall rpcCall;
private final Message param;
private Message param;
private final String clientAddress;
private final long responseSize;
private final long blockBytesScanned;
Expand All @@ -47,7 +54,6 @@ public RpcLogDetails(RpcCall rpcCall, Message param, String clientAddress, long
long blockBytesScanned, String className, boolean isSlowLog, boolean isLargeLog) {
super(SLOW_LOG_EVENT);
this.rpcCall = rpcCall;
this.param = param;
this.clientAddress = clientAddress;
this.responseSize = responseSize;
this.blockBytesScanned = blockBytesScanned;
Expand All @@ -60,6 +66,40 @@ public RpcLogDetails(RpcCall rpcCall, Message param, String clientAddress, long
// would result in corrupted attributes
this.connectionAttributes = rpcCall.getConnectionAttributes();
this.requestAttributes = rpcCall.getRequestAttributes();

// We also need to deep copy the message because the CodedInputStream may be
// overwritten before this slow log is consumed. Such overwriting could
// cause the slow log payload to be corrupt
try {
if (param instanceof ClientProtos.ScanRequest) {
ClientProtos.ScanRequest scanRequest = (ClientProtos.ScanRequest) param;
this.param = ClientProtos.ScanRequest.parseFrom(scanRequest.toByteArray());
} else if (param instanceof ClientProtos.MutationProto) {
ClientProtos.MutationProto mutationProto = (ClientProtos.MutationProto) param;
this.param = ClientProtos.MutationProto.parseFrom(mutationProto.toByteArray());
} else if (param instanceof ClientProtos.GetRequest) {
ClientProtos.GetRequest getRequest = (ClientProtos.GetRequest) param;
this.param = ClientProtos.GetRequest.parseFrom(getRequest.toByteArray());
} else if (param instanceof ClientProtos.MultiRequest) {
ClientProtos.MultiRequest multiRequest = (ClientProtos.MultiRequest) param;
this.param = ClientProtos.MultiRequest.parseFrom(multiRequest.toByteArray());
} else if (param instanceof ClientProtos.MutateRequest) {
ClientProtos.MutateRequest mutateRequest = (ClientProtos.MutateRequest) param;
this.param = ClientProtos.MutateRequest.parseFrom(mutateRequest.toByteArray());
} else if (param instanceof ClientProtos.CoprocessorServiceRequest) {
ClientProtos.CoprocessorServiceRequest coprocessorServiceRequest =
(ClientProtos.CoprocessorServiceRequest) param;
this.param =
ClientProtos.CoprocessorServiceRequest.parseFrom(coprocessorServiceRequest.toByteArray());
} else {
this.param = param;
}
} catch (InvalidProtocolBufferException e) {
LOG.error("Failed to parse protobuf for message {}", param, e);
if (this.param == null) {
Copy link
Contributor

@virajjasani virajjasani Nov 6, 2023

Choose a reason for hiding this comment

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

this would likely always be true right?

Copy link
Contributor Author

@rmdmattingly rmdmattingly Nov 6, 2023

Choose a reason for hiding this comment

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

yeah, I can't imagine when it wouldn't be true so maybe we should just remove this

this.param = param;
}
}
}

public RpcCall getRpcCall() {
Expand Down