rmdmattingly commented on code in PR #5481:
URL: https://github.com/apache/hbase/pull/5481#discussion_r1373417312
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/SlowLogParams.java:
##########
@@ -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();.isEquals();
Review Comment:
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
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/namequeues/RpcLogDetails.java:
##########
@@ -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) {
+ this.param = param;
+ }
+ }
Review Comment:
This is kind of ugly and I'm open to other suggestions, but it feels like a
sad inevitability when dealing with an ambiguous proto Message that we must
deep copy. I'm also (de)serializing here because just using protobuf's
`mergeFrom` only provides a shallow copy and consequently does not resolve the
corruption issue.
--
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]
