HBASE-27536: Include more request information in slowlog for Scans#5155
Merged
bbeaudreault merged 4 commits intoapache:masterfrom Apr 16, 2023
Merged
HBASE-27536: Include more request information in slowlog for Scans#5155bbeaudreault merged 4 commits intoapache:masterfrom
bbeaudreault merged 4 commits intoapache:masterfrom
Conversation
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
rmdmattingly
force-pushed
the
rmattingly/HBASE-27536
branch
from
3dbe08c to
f99664d
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
rmdmattingly
force-pushed
the
rmattingly/HBASE-27536
branch
from
f99664d to
5c3a47e
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
I'd be surprised if this changeset caused these unit test failures 🤔 |
Contributor
|
The test failures don't seem relevant. I have retriggered the build.
I will start the code review tomorrow, sorry for the delay. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
virajjasani
reviewed
Apr 5, 2023
Contributor
virajjasani
left a comment
virajjasani
left a comment
There was a problem hiding this comment.
Overall looks good.
How about adding this?
--- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/OnlineLogRecord.java
+++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/OnlineLogRecord.java
@@ -166,7 +166,7 @@ final public class OnlineLogRecord extends LogEntry {
final long responseSize, final long blockBytesScanned, 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 Optional<Scan> scan) {
+ final int multiMutationsCount, final int multiServiceCalls, final Scan scan) {
this.startTime = startTime;
this.processingTime = processingTime;
this.queueTime = queueTime;
@@ -182,7 +182,7 @@ final public class OnlineLogRecord extends LogEntry {
this.multiGetsCount = multiGetsCount;
this.multiMutationsCount = multiMutationsCount;
this.multiServiceCalls = multiServiceCalls;
- this.scan = scan;
+ this.scan = Optional.of(scan);
}
public static class OnlineLogRecordBuilder {
@@ -201,7 +201,7 @@ final public class OnlineLogRecord extends LogEntry {
private int multiGetsCount;
private int multiMutationsCount;
private int multiServiceCalls;
- private Optional<Scan> scan = Optional.empty();
+ private Scan scan = null;
public OnlineLogRecordBuilder setStartTime(long startTime) {
this.startTime = startTime;
@@ -282,7 +282,7 @@ final public class OnlineLogRecord extends LogEntry {
}
public OnlineLogRecordBuilder setScan(Scan scan) {
- this.scan = Optional.of(scan);
+ this.scan = scan;
return this;
}
--- a/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestOnlineLogRecord.java
+++ b/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestOnlineLogRecord.java
@@ -49,7 +49,7 @@ public class TestOnlineLogRecord {
+ " \"maxVersions\": 1,\n" + " \"timeRange\": [\n" + " \"0\",\n"
+ " \"9223372036854775807\"\n" + " ]\n" + " }\n" + "}";
OnlineLogRecord o = new OnlineLogRecord(1, 2, 3, 4, 5, null, null, null, null, null, null, null,
- 6, 7, 0, Optional.of(scan));
+ 6, 7, 0, scan);
String actualOutput = o.toJsonPrettyPrint();
Assert.assertEquals(actualOutput, expectedOutput);
}
| try { | ||
| jsonObj.add("scan", JsonParser.parseString(slowLogPayload.getScan().get().toJSON())); | ||
| } catch (IOException e) { | ||
| LOG.warn("Failed to serialize scan {}", slowLogPayload.getScan().get(), e); |
Contributor
There was a problem hiding this comment.
Will this print byte[] ?
Contributor
Author
There was a problem hiding this comment.
I believe it would manifest as the toString representation. For example, this:
import org.apache.hadoop.hbase.client.Scan;
public static void main(String[] args) {
Scan scan = new Scan();
scan.withStartRow(Bytes.toBytes("1234"));
LOG.info("Failed to serialize scan {}", scan);
}produces this:
Failed to serialize scan {"startRow":"1234","stopRow":"","batch":-1,"cacheBlocks":true,"totalColumns":0,"maxResultSize":"-1","families":{},"caching":-1,"maxVersions":1,"timeRange":["0","9223372036854775807"]}
hbase-client/src/main/java/org/apache/hadoop/hbase/client/OnlineLogRecord.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
@rmdmattingly could you please also create branch-2 PR so both can be merged together? @bbeaudreault sir, anything from your side? |
bbeaudreault
reviewed
Apr 7, 2023
Contributor
bbeaudreault
left a comment
bbeaudreault
left a comment
There was a problem hiding this comment.
Sorry I thought I had left a comment, but it was left pending on accident.
Not a huge deal but the only thing I saw, maybe a chance to minor optimize the json serialization
| } | ||
| if (slowLogPayload.getScan().isPresent()) { | ||
| try { | ||
| jsonObj.add("scan", JsonParser.parseString(slowLogPayload.getScan().get().toJSON())); |
Contributor
There was a problem hiding this comment.
Rather than toJson, then parse, then back to json. What I'd you just use toMap() here and let the local gson handle turning that into json?
I believe toJson just does toMap and then converts that to a json string.
Contributor
Author
There was a problem hiding this comment.
We have to add a JsonElement either way, so I think with toMap we'd still have to convert the map to json here
Contributor
There was a problem hiding this comment.
Can't you do this?
jsonObj.add("scan", gson.toJsonTree(scan.toMap()));
I think this is categorically better than JsonParser.parseString(slowLogPayload.getScan().get().toJSON()). The reason is because the current approach requires going scan -> map -> JsonElement -> string -> JsonElement -> String (the latter two coming from JsonParser and then the serialization of the resulting jsonObj. The approach recommended above just has to go scan -> map -> JsonElement -> string.
Contributor
Author
There was a problem hiding this comment.
Oh yeah, you're right. Will make this change! Thanks for the tip
Contributor
Author
|
And yes, I'll definitely create a branch-2 PR either this weekend or first thing next week! Thanks for the review here both! |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
rmdmattingly
force-pushed
the
rmattingly/HBASE-27536
branch
from
4ccfbe9 to
2b79635
Compare
|
🎊 +1 overall
This message was automatically generated. |
Contributor
|
@rmdmattingly can you clean up the javac warning? looks like just an unused variable |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
bbeaudreault
approved these changes
Apr 11, 2023
Contributor
|
Sorry, I could not get to my desk unfortunately hence will mostly merge this tomorrow morning unless it is already merged. |
virajjasani
changed the title
Contributor
|
@virajjasani don't worry, I can take care of it. We're doing a quick end-to-end test internally before merging, just to make sure there aren't any small tweaks to make before committing. I'll merge this and the backport once that checks out. |
Contributor
|
Thanks a lot, as always!! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
HBASE-27536
Currently slow logs only includes a barebones text format of the underlying protobuf Message fields. Gets and Puts are relatively straightforward from a query cost perspective, but the configuration of Scans can have very significant performance implications.
With that in mind, we think it would be valuable to include the JSON representation of a given Scan to the slow log that it may produce.
cc @bbeaudreault @virajjasani