Skip to content
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

HBASE-27536: improve slowlog payload #4937

Closed
wants to merge 1 commit into from
Closed

HBASE-27536: improve slowlog payload #4937

Show file tree
Hide file tree
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
121 changes: 111 additions & 10 deletions hbase-client/src/main/java/org/apache/hadoop/hbase/client/OnlineLogRecord.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,22 @@
*/
package org.apache.hadoop.hbase.client;

import java.util.List;
import java.util.Optional;
import org.apache.commons.lang3.builder.EqualsBuilder;
import org.apache.commons.lang3.builder.HashCodeBuilder;
import org.apache.commons.lang3.builder.ToStringBuilder;
import org.apache.hadoop.hbase.HConstants;
import org.apache.hadoop.hbase.util.GsonUtil;
import org.apache.yetus.audience.InterfaceAudience;
import org.apache.yetus.audience.InterfaceStability;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import org.apache.hbase.thirdparty.com.google.gson.Gson;
import org.apache.hbase.thirdparty.com.google.gson.JsonElement;
import org.apache.hbase.thirdparty.com.google.gson.JsonObject;
import org.apache.hbase.thirdparty.com.google.gson.JsonParser;
import org.apache.hbase.thirdparty.com.google.gson.JsonSerializer;

/**
Expand All @@ -36,13 +43,31 @@
@InterfaceStability.Evolving
final public class OnlineLogRecord extends LogEntry {

// used to convert object to pretty printed format
// used by toJsonPrettyPrint()
private static final Logger LOG = LoggerFactory.getLogger(OnlineLogRecord.class.getName());
private static final JsonElement EXCLUDED_NODE = JsonParser.parseString(HConstants.EMPTY_STRING);

private static JsonElement serializeCatchAll(Operation operation) {
try {
return JsonParser.parseString(operation.toJSON());
} catch (Exception e) {
LOG.warn("Suppressing exception during OnlineLogRecord serialization with operation {}",
operation, e);
return EXCLUDED_NODE;
}
}

private static final Gson INNER_GSON = GsonUtil.createGson().setPrettyPrinting()
.registerTypeAdapter(Operation.class,
(JsonSerializer<
Operation>) (operation, type, jsonSerializationContext) -> serializeCatchAll(operation))
.registerTypeAdapter(Optional.class,
(JsonSerializer<Optional<?>>) (optional, type, jsonSerializationContext) -> optional
.map(jsonSerializationContext::serialize).orElse(EXCLUDED_NODE))
.create();
private static final Gson GSON =
GsonUtil.createGson().setPrettyPrinting().registerTypeAdapter(OnlineLogRecord.class,
(JsonSerializer<OnlineLogRecord>) (slowLogPayload, type, jsonSerializationContext) -> {
Gson gson = new Gson();
JsonObject jsonObj = (JsonObject) gson.toJsonTree(slowLogPayload);
JsonObject jsonObj = (JsonObject) INNER_GSON.toJsonTree(slowLogPayload);
Copy link
Contributor

Choose a reason for hiding this comment

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

did you try doing jsonSerliazationContext.serialize(slowPayLoad)? I feel like that's the appropriate call which should hopefully work with chaining, but not sure.

if (slowLogPayload.getMultiGetsCount() == 0) {
jsonObj.remove("multiGetsCount");
}
Expand Down Expand Up @@ -71,6 +96,10 @@ final public class OnlineLogRecord extends LogEntry {
private final int multiGetsCount;
private final int multiMutationsCount;
private final int multiServiceCalls;
private final Optional<Scan> scan;
private final Optional<List<Operation>> multi;
private final Optional<Get> get;
private final Optional<Mutation> mutate;

public long getStartTime() {
return startTime;
Expand Down Expand Up @@ -128,11 +157,52 @@ public int getMultiServiceCalls() {
return multiServiceCalls;
}

private OnlineLogRecord(final long startTime, final int processingTime, final int queueTime,
/**
* If {@value org.apache.hadoop.hbase.HConstants#SLOW_LOG_OPERATION_MESSAGE_PAYLOAD_ENABLED} is
* enabled then this value may be present and should represent the Scan that produced the given
* {@link OnlineLogRecord}. This value should only be present if {@link #getMulti()},
* {@link #getGet()}, and {@link #getMutate()} are empty
*/
public Optional<Scan> getScan() {
return scan;
}

/**
* If {@value org.apache.hadoop.hbase.HConstants#SLOW_LOG_OPERATION_MESSAGE_PAYLOAD_ENABLED} is
* enabled then this value may be present and should represent the MultiRequest that produced the
* given {@link OnlineLogRecord}. This value should only be present if {@link #getScan},
* {@link #getGet()}, and {@link #getMutate()} are empty
*/
public Optional<List<Operation>> getMulti() {
return multi;
}

/**
* If {@value org.apache.hadoop.hbase.HConstants#SLOW_LOG_OPERATION_MESSAGE_PAYLOAD_ENABLED} is
* enabled then this value may be present and should represent the Get that produced the given
* {@link OnlineLogRecord}. This value should only be present if {@link #getScan()},
* {@link #getMulti()} ()}, and {@link #getMutate()} are empty
*/
public Optional<Get> getGet() {
return get;
}

/**
* If {@value org.apache.hadoop.hbase.HConstants#SLOW_LOG_OPERATION_MESSAGE_PAYLOAD_ENABLED} is
* enabled then this value may be present and should represent the Mutation that produced the
* given {@link OnlineLogRecord}. This value should only be present if {@link #getScan},
* {@link #getMulti()} ()}, and {@link #getGet()} ()} are empty
*/
public Optional<Mutation> getMutate() {
return mutate;
}
Comment on lines +160 to +198
Copy link
Contributor

Choose a reason for hiding this comment

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

We should think a little about whether this is actually the API we want to expose. Since this class is InterfaceAudience.Public, any changes we make here we're stuck with for a long time. We can only remove/rename methods (i.e. breaking changes) at major version releases and we haven't had a major version release in years.

There's potentially a little more flexibility since it's InterfaceStability.Evolving, but I don't think there's really consensus across all committers/PMC as to whether this grants us anything. It's not mentioned in our guides except:

Public packages marked as evolving may be changed, but it is discouraged.

Anyway, not saying what we have here is wrong per-se. But we should take a minute to think about how we might evolve usage of these slow log stuff over time and make sure what we have here will support that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is good advice for sure. I've mulled it over a little, and I'm not sure what I'd change at this level. Please let me know if you have any ideas


OnlineLogRecord(final long startTime, final int processingTime, final int queueTime,
final long responseSize, final String clientAddress, final String serverClass,
final String methodName, final String callDetails, final String param, final String regionName,
final String userName, final int multiGetsCount, final int multiMutationsCount,
final int multiServiceCalls) {
final int multiServiceCalls, final Optional<Scan> scan, final Optional<List<Operation>> multi,
final Optional<Get> get, final Optional<Mutation> mutate) {
this.startTime = startTime;
this.processingTime = processingTime;
this.queueTime = queueTime;
Expand All @@ -147,6 +217,10 @@ private OnlineLogRecord(final long startTime, final int processingTime, final in
this.multiGetsCount = multiGetsCount;
this.multiMutationsCount = multiMutationsCount;
this.multiServiceCalls = multiServiceCalls;
this.scan = scan;
this.multi = multi;
this.get = get;
this.mutate = mutate;
}

public static class OnlineLogRecordBuilder {
Expand All @@ -164,6 +238,10 @@ public static class OnlineLogRecordBuilder {
private int multiGetsCount;
private int multiMutationsCount;
private int multiServiceCalls;
private Optional<Scan> scan = Optional.empty();
private Optional<List<Operation>> multi = Optional.empty();
private Optional<Get> get = Optional.empty();
private Optional<Mutation> mutate = Optional.empty();

public OnlineLogRecordBuilder setStartTime(long startTime) {
this.startTime = startTime;
Expand Down Expand Up @@ -235,10 +313,30 @@ public OnlineLogRecordBuilder setMultiServiceCalls(int multiServiceCalls) {
return this;
}

public OnlineLogRecordBuilder setScan(Scan scan) {
this.scan = Optional.of(scan);
return this;
}

public OnlineLogRecordBuilder setMulti(List<Operation> multi) {
this.multi = Optional.of(multi);
return this;
}

public OnlineLogRecordBuilder setGet(Get get) {
this.get = Optional.of(get);
return this;
}

public OnlineLogRecordBuilder setMutate(Mutation mutate) {
this.mutate = Optional.of(mutate);
return this;
}

public OnlineLogRecord build() {
return new OnlineLogRecord(startTime, processingTime, queueTime, responseSize, clientAddress,
serverClass, methodName, callDetails, param, regionName, userName, multiGetsCount,
multiMutationsCount, multiServiceCalls);
multiMutationsCount, multiServiceCalls, scan, multi, get, mutate);
}
}

Expand All @@ -261,15 +359,17 @@ public boolean equals(Object o) {
.append(multiServiceCalls, that.multiServiceCalls).append(clientAddress, that.clientAddress)
.append(serverClass, that.serverClass).append(methodName, that.methodName)
.append(callDetails, that.callDetails).append(param, that.param)
.append(regionName, that.regionName).append(userName, that.userName).isEquals();
.append(regionName, that.regionName).append(userName, that.userName).append(scan, that.scan)
.append(multi, that.multi).append(get, that.get).append(mutate, that.mutate).isEquals();
}

@Override
public int hashCode() {
return new HashCodeBuilder(17, 37).append(startTime).append(processingTime).append(queueTime)
.append(responseSize).append(clientAddress).append(serverClass).append(methodName)
.append(callDetails).append(param).append(regionName).append(userName).append(multiGetsCount)
.append(multiMutationsCount).append(multiServiceCalls).toHashCode();
.append(multiMutationsCount).append(multiServiceCalls).append(scan).append(multi).append(get)
.append(mutate).toHashCode();
}

@Override
rmdmattingly marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -286,7 +386,8 @@ public String toString() {
.append("callDetails", callDetails).append("param", param).append("regionName", regionName)
.append("userName", userName).append("multiGetsCount", multiGetsCount)
.append("multiMutationsCount", multiMutationsCount)
.append("multiServiceCalls", multiServiceCalls).toString();
.append("multiServiceCalls", multiServiceCalls).append("scan", scan).append("multi", multi)
.append("get", get).append("mutate", mutate).toString();
}

}
73 changes: 71 additions & 2 deletions 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 @@ -23,6 +23,8 @@
import org.apache.commons.lang3.builder.ToStringBuilder;
import org.apache.yetus.audience.InterfaceAudience;

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

/**
* SlowLog params object that contains detailed info as params and region name : to be used for
* filter purpose
Expand All @@ -32,15 +34,63 @@ public class SlowLogParams {

private final String regionName;
private final String params;
private final ClientProtos.Scan scan;
private final ClientProtos.MultiRequest multi;
private final ClientProtos.Get get;
private final ClientProtos.MutationProto mutate;

public SlowLogParams(String regionName, String params, ClientProtos.Scan scan) {
this.regionName = regionName;
this.params = params;
this.scan = scan;
this.multi = null;
this.get = null;
this.mutate = null;
}

public SlowLogParams(String regionName, String params, ClientProtos.MultiRequest multi) {
this.regionName = regionName;
this.params = params;
this.scan = null;
this.multi = multi;
this.get = null;
this.mutate = null;
}

public SlowLogParams(String regionName, String params, ClientProtos.Get get) {
this.regionName = regionName;
this.params = params;
this.scan = null;
this.multi = null;
this.get = get;
this.mutate = null;
}

public SlowLogParams(String regionName, String params, ClientProtos.MutationProto mutate) {
this.regionName = regionName;
this.params = params;
this.scan = null;
this.multi = null;
this.get = null;
this.mutate = mutate;
}

public SlowLogParams(String regionName, String params) {
this.regionName = regionName;
this.params = params;
this.scan = null;
this.multi = null;
this.get = null;
this.mutate = null;
}

public SlowLogParams(String params) {
this.regionName = StringUtils.EMPTY;
this.params = params;
this.scan = null;
this.multi = null;
this.get = null;
this.mutate = null;
}

public String getRegionName() {
Expand All @@ -51,9 +101,26 @@ public String getParams() {
return params;
}

public ClientProtos.Scan getScan() {
return scan;
}

public ClientProtos.MultiRequest getMulti() {
return multi;
}

public ClientProtos.Get getGet() {
return get;
}

public ClientProtos.MutationProto getMutate() {
return mutate;
}

@Override
public String toString() {
return new ToStringBuilder(this).append("regionName", regionName).append("params", params)
.append("scan", scan).append("multi", multi).append("get", get).append("mutate", mutate)
.toString();
}

Expand All @@ -67,11 +134,13 @@ public boolean equals(Object o) {
}
SlowLogParams that = (SlowLogParams) o;
return new EqualsBuilder().append(regionName, that.regionName).append(params, that.params)
.isEquals();
.append(scan, that.scan).append(multi, that.multi).append(get, that.get)
.append(mutate, that.mutate).isEquals();
}

@Override
public int hashCode() {
return new HashCodeBuilder(17, 37).append(regionName).append(params).toHashCode();
return new HashCodeBuilder(17, 37).append(regionName).append(params).append(scan).append(multi)
.append(get).append(mutate).toHashCode();
}
}
Loading