From 1b20c920f4b0a1ffec36856972adaca16626e8b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1vid=20Paksy?= Date: Mon, 30 Sep 2024 11:15:04 +0200 Subject: [PATCH 1/5] HBASE-28865 Implement proper string builder for MoveRegionRequest in ProtobufUtil.getShortTextFormat --- .../hbase/shaded/protobuf/ProtobufUtil.java | 7 +- .../shaded/protobuf/TestProtobufUtil.java | 148 ++++++++++++++++++ 2 files changed, 154 insertions(+), 1 deletion(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java index 427ad98f7fa5..54e633345e89 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java @@ -2150,7 +2150,9 @@ public static RegionEventDescriptor toRegionEventDescriptor(EventType eventType, * @return toString of passed m */ public static String getShortTextFormat(Message m) { - if (m == null) return "null"; + if (m == null) { + return "null"; + } if (m instanceof ScanRequest) { // This should be small and safe to output. No data. return TextFormat.shortDebugString(m); @@ -2192,6 +2194,9 @@ public static String getShortTextFormat(Message m) { ClientProtos.CoprocessorServiceRequest r = (ClientProtos.CoprocessorServiceRequest) m; return "coprocessorService= " + r.getCall().getServiceName() + ":" + r.getCall().getMethodName(); + } else if (m instanceof MasterProtos.MoveRegionRequest) { + // This should be small and safe to output. No data. + return TextFormat.shortDebugString(m); } return "TODO: " + m.getClass().toString(); } diff --git a/hbase-client/src/test/java/org/apache/hadoop/hbase/shaded/protobuf/TestProtobufUtil.java b/hbase-client/src/test/java/org/apache/hadoop/hbase/shaded/protobuf/TestProtobufUtil.java index ee0a711634de..2debb75d43c6 100644 --- a/hbase-client/src/test/java/org/apache/hadoop/hbase/shaded/protobuf/TestProtobufUtil.java +++ b/hbase-client/src/test/java/org/apache/hadoop/hbase/shaded/protobuf/TestProtobufUtil.java @@ -57,6 +57,7 @@ import org.apache.hbase.thirdparty.com.google.protobuf.ByteString; import org.apache.hbase.thirdparty.com.google.protobuf.BytesValue; import org.apache.hbase.thirdparty.com.google.protobuf.InvalidProtocolBufferException; +import org.apache.hbase.thirdparty.com.google.protobuf.UnsafeByteOperations; import org.apache.hadoop.hbase.shaded.protobuf.generated.CellProtos; import org.apache.hadoop.hbase.shaded.protobuf.generated.ClientProtos; @@ -69,7 +70,9 @@ import org.apache.hadoop.hbase.shaded.protobuf.generated.HBaseProtos; import org.apache.hadoop.hbase.shaded.protobuf.generated.HBaseProtos.NameBytesPair; import org.apache.hadoop.hbase.shaded.protobuf.generated.LockServiceProtos; +import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProtos; import org.apache.hadoop.hbase.shaded.protobuf.generated.ProcedureProtos; +import org.apache.hadoop.hbase.shaded.protobuf.generated.RegionServerStatusProtos; @Category(SmallTests.class) public class TestProtobufUtil { @@ -623,4 +626,149 @@ public void testSlowLogParamsMutateRequest() { assertTrue(slowLogParams.getParams() .contains(Bytes.toStringBinary(mutationProto.getRow().toByteArray()))); } + + @Test + public void testGetShortTextFormatNull() { + String actual = ProtobufUtil.getShortTextFormat(null); + assertNotNull(actual); + assertEquals("null", actual); + } + + @Test + public void testGetShortTextFormatScanRequest() { + ClientProtos.ScanRequest.Builder builder = ClientProtos.ScanRequest.newBuilder(); + builder.setRegion(givenRegion()); + ClientProtos.ScanRequest scanRequest = builder.build(); + + String actual = ProtobufUtil.getShortTextFormat(scanRequest); + + assertNotNull(actual); + assertEquals("region { type: REGION_NAME value: \"test\" }", actual); + } + + @Test + public void testGetShortTextFormatRegionServerReportRequest() { + RegionServerStatusProtos.RegionServerReportRequest.Builder builder = + RegionServerStatusProtos.RegionServerReportRequest.newBuilder(); + builder.setServer(givenServerName()); + RegionServerStatusProtos.RegionServerReportRequest request = builder.build(); + + String actual = ProtobufUtil.getShortTextFormat(request); + + assertNotNull(actual); + assertEquals("server host_name: \"a.b.com\" load { numberOfRequests: 0 }", actual); + } + + @Test + public void testGetShortTextFormatRegionServerStartupRequest() { + RegionServerStatusProtos.RegionServerStartupRequest.Builder builder = + RegionServerStatusProtos.RegionServerStartupRequest.newBuilder(); + builder.setPort(8080); + builder.setServerCurrentTime(111111L); + builder.setServerStartCode(15L); + builder.setUseThisHostnameInstead("some-host-name"); + RegionServerStatusProtos.RegionServerStartupRequest regionServerStartupRequest = + builder.build(); + + String actual = ProtobufUtil.getShortTextFormat(regionServerStartupRequest); + + assertNotNull(actual); + assertEquals( + "port: 8080 server_start_code: 15 server_current_time: 111111 use_this_hostname_instead: \"some-host-name\"", + actual); + } + + @Test + public void testGetShortTextFormatMutationProto() { + MutationProto mutationProto = + ClientProtos.MutationProto.newBuilder().setRow(ByteString.copyFromUtf8("row123")).build(); + + String actual = ProtobufUtil.getShortTextFormat(mutationProto); + + assertNotNull(actual); + assertEquals("row=row123, type=APPEND", actual); + } + + @Test + public void testGetShortTextFormatGetRequest() throws IOException { + ClientProtos.GetRequest getRequest = ClientProtos.GetRequest.newBuilder() + .setRegion(givenRegion()).setGet(ProtobufUtil.toGet(new Get(Bytes.toBytes("foo")))).build(); + + String actual = ProtobufUtil.getShortTextFormat(getRequest); + + assertNotNull(actual); + assertEquals("region= test, row=foo", actual); + } + + @Test + public void testGetShortTextFormatMultiRequest() throws IOException { + ClientProtos.Action action = ClientProtos.Action.newBuilder() + .setGet(ProtobufUtil.toGet(new Get(Bytes.toBytes("foo")))).build(); + ClientProtos.RegionAction regionAction = + ClientProtos.RegionAction.newBuilder().addAction(action).setRegion(givenRegion()).build(); + + ClientProtos.MultiRequest multiRequest = + ClientProtos.MultiRequest.newBuilder().addRegionAction(regionAction).build(); + + String actual = ProtobufUtil.getShortTextFormat(multiRequest); + + assertNotNull(actual); + assertEquals("region= test, for 1 action(s) and 1st row key=foo", actual); + } + + @Test + public void testGetShortTextFormatMutateRequest() throws IOException { + ClientProtos.MutateRequest mutateRequest = ClientProtos.MutateRequest.newBuilder() + .setMutation( + ProtobufUtil.toMutation(MutationType.INCREMENT, new Increment(Bytes.toBytes("foo")))) + .setRegion(givenRegion()).build(); + + String actual = ProtobufUtil.getShortTextFormat(mutateRequest); + + assertNotNull(actual); + assertEquals("region= test, row=foo", actual); + } + + @Test + public void testGetShortTextFormatCoprocessorServiceRequest() { + ClientProtos.CoprocessorServiceCall call = ClientProtos.CoprocessorServiceCall.newBuilder() + .setRow(UnsafeByteOperations.unsafeWrap(Bytes.toBytes("foo"))).setMethodName("awesomeMethod") + .setServiceName("awesomeService") + .setRequest(UnsafeByteOperations.unsafeWrap(Bytes.toBytes("foo-request"))).build(); + + ClientProtos.CoprocessorServiceRequest.Builder builder = + ClientProtos.CoprocessorServiceRequest.newBuilder(); + builder.setRegion(givenRegion()); + builder.setCall(call); + ClientProtos.CoprocessorServiceRequest coprocessorServiceRequest = builder.build(); + + String actual = ProtobufUtil.getShortTextFormat(coprocessorServiceRequest); + + assertNotNull(actual); + assertEquals("coprocessorService= awesomeService:awesomeMethod", actual); + } + + @Test + public void testGetShortTextFormatMoveRegionRequest() { + MasterProtos.MoveRegionRequest.Builder builder = MasterProtos.MoveRegionRequest.newBuilder(); + builder.setRegion(givenRegion()); + builder.setDestServerName(givenServerName()); + MasterProtos.MoveRegionRequest moveRegionRequest = builder.build(); + + String actual = ProtobufUtil.getShortTextFormat(moveRegionRequest); + + assertNotNull(actual); + assertEquals( + "region { type: REGION_NAME value: \"test\" } dest_server_name { host_name: \"a.b.com\" }", + actual); + } + + private static HBaseProtos.ServerName givenServerName() { + return HBaseProtos.ServerName.newBuilder().setHostName("a.b.com").build(); + } + + private static HBaseProtos.RegionSpecifier givenRegion() { + return HBaseProtos.RegionSpecifier.newBuilder().setValue(ByteString.copyFromUtf8("test")) + .setType(HBaseProtos.RegionSpecifier.RegionSpecifierType.REGION_NAME).build(); + } } From f0858a380b59caf603b5075d9a9854313c531e5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1vid=20Paksy?= Date: Wed, 9 Oct 2024 15:56:37 +0200 Subject: [PATCH 2/5] HBASE-28865: Removed copy-paste comment from ProtobufUtil --- .../org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java | 1 - 1 file changed, 1 deletion(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java index 54e633345e89..e9eb085ee7bd 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java @@ -2195,7 +2195,6 @@ public static String getShortTextFormat(Message m) { return "coprocessorService= " + r.getCall().getServiceName() + ":" + r.getCall().getMethodName(); } else if (m instanceof MasterProtos.MoveRegionRequest) { - // This should be small and safe to output. No data. return TextFormat.shortDebugString(m); } return "TODO: " + m.getClass().toString(); From af41129686e56d48246294ad1d86b149357e9971 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1vid=20Paksy?= Date: Wed, 9 Oct 2024 15:57:32 +0200 Subject: [PATCH 3/5] HBASE-28865: Define test class constants instead of static methods --- .../shaded/protobuf/TestProtobufUtil.java | 32 ++++++++----------- 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/hbase-client/src/test/java/org/apache/hadoop/hbase/shaded/protobuf/TestProtobufUtil.java b/hbase-client/src/test/java/org/apache/hadoop/hbase/shaded/protobuf/TestProtobufUtil.java index 2debb75d43c6..38e628dc1f2a 100644 --- a/hbase-client/src/test/java/org/apache/hadoop/hbase/shaded/protobuf/TestProtobufUtil.java +++ b/hbase-client/src/test/java/org/apache/hadoop/hbase/shaded/protobuf/TestProtobufUtil.java @@ -81,6 +81,11 @@ public class TestProtobufUtil { HBaseClassTestRule.forClass(TestProtobufUtil.class); private static final String TAG_STR = "tag-1"; private static final byte TAG_TYPE = (byte) 10; + private static final HBaseProtos.ServerName SERVER_NAME = + HBaseProtos.ServerName.newBuilder().setHostName("a.b.com").build(); + private static final HBaseProtos.RegionSpecifier REGION = + HBaseProtos.RegionSpecifier.newBuilder().setValue(ByteString.copyFromUtf8("test")) + .setType(HBaseProtos.RegionSpecifier.RegionSpecifierType.REGION_NAME).build(); public TestProtobufUtil() { } @@ -637,7 +642,7 @@ public void testGetShortTextFormatNull() { @Test public void testGetShortTextFormatScanRequest() { ClientProtos.ScanRequest.Builder builder = ClientProtos.ScanRequest.newBuilder(); - builder.setRegion(givenRegion()); + builder.setRegion(REGION); ClientProtos.ScanRequest scanRequest = builder.build(); String actual = ProtobufUtil.getShortTextFormat(scanRequest); @@ -650,7 +655,7 @@ public void testGetShortTextFormatScanRequest() { public void testGetShortTextFormatRegionServerReportRequest() { RegionServerStatusProtos.RegionServerReportRequest.Builder builder = RegionServerStatusProtos.RegionServerReportRequest.newBuilder(); - builder.setServer(givenServerName()); + builder.setServer(SERVER_NAME); RegionServerStatusProtos.RegionServerReportRequest request = builder.build(); String actual = ProtobufUtil.getShortTextFormat(request); @@ -691,8 +696,8 @@ public void testGetShortTextFormatMutationProto() { @Test public void testGetShortTextFormatGetRequest() throws IOException { - ClientProtos.GetRequest getRequest = ClientProtos.GetRequest.newBuilder() - .setRegion(givenRegion()).setGet(ProtobufUtil.toGet(new Get(Bytes.toBytes("foo")))).build(); + ClientProtos.GetRequest getRequest = ClientProtos.GetRequest.newBuilder().setRegion(REGION) + .setGet(ProtobufUtil.toGet(new Get(Bytes.toBytes("foo")))).build(); String actual = ProtobufUtil.getShortTextFormat(getRequest); @@ -705,7 +710,7 @@ public void testGetShortTextFormatMultiRequest() throws IOException { ClientProtos.Action action = ClientProtos.Action.newBuilder() .setGet(ProtobufUtil.toGet(new Get(Bytes.toBytes("foo")))).build(); ClientProtos.RegionAction regionAction = - ClientProtos.RegionAction.newBuilder().addAction(action).setRegion(givenRegion()).build(); + ClientProtos.RegionAction.newBuilder().addAction(action).setRegion(REGION).build(); ClientProtos.MultiRequest multiRequest = ClientProtos.MultiRequest.newBuilder().addRegionAction(regionAction).build(); @@ -721,7 +726,7 @@ public void testGetShortTextFormatMutateRequest() throws IOException { ClientProtos.MutateRequest mutateRequest = ClientProtos.MutateRequest.newBuilder() .setMutation( ProtobufUtil.toMutation(MutationType.INCREMENT, new Increment(Bytes.toBytes("foo")))) - .setRegion(givenRegion()).build(); + .setRegion(REGION).build(); String actual = ProtobufUtil.getShortTextFormat(mutateRequest); @@ -738,7 +743,7 @@ public void testGetShortTextFormatCoprocessorServiceRequest() { ClientProtos.CoprocessorServiceRequest.Builder builder = ClientProtos.CoprocessorServiceRequest.newBuilder(); - builder.setRegion(givenRegion()); + builder.setRegion(REGION); builder.setCall(call); ClientProtos.CoprocessorServiceRequest coprocessorServiceRequest = builder.build(); @@ -751,8 +756,8 @@ public void testGetShortTextFormatCoprocessorServiceRequest() { @Test public void testGetShortTextFormatMoveRegionRequest() { MasterProtos.MoveRegionRequest.Builder builder = MasterProtos.MoveRegionRequest.newBuilder(); - builder.setRegion(givenRegion()); - builder.setDestServerName(givenServerName()); + builder.setRegion(REGION); + builder.setDestServerName(SERVER_NAME); MasterProtos.MoveRegionRequest moveRegionRequest = builder.build(); String actual = ProtobufUtil.getShortTextFormat(moveRegionRequest); @@ -762,13 +767,4 @@ public void testGetShortTextFormatMoveRegionRequest() { "region { type: REGION_NAME value: \"test\" } dest_server_name { host_name: \"a.b.com\" }", actual); } - - private static HBaseProtos.ServerName givenServerName() { - return HBaseProtos.ServerName.newBuilder().setHostName("a.b.com").build(); - } - - private static HBaseProtos.RegionSpecifier givenRegion() { - return HBaseProtos.RegionSpecifier.newBuilder().setValue(ByteString.copyFromUtf8("test")) - .setType(HBaseProtos.RegionSpecifier.RegionSpecifierType.REGION_NAME).build(); - } } From 05ea9f46373d790dca399ec9b884c480d3dbf695 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1vid=20Paksy?= Date: Thu, 10 Oct 2024 09:04:41 +0200 Subject: [PATCH 4/5] HBASE-28865: Use ByteString.copyFrom instead of UnsafeByteOperations.unsafeWrap --- .../hadoop/hbase/shaded/protobuf/TestProtobufUtil.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/hbase-client/src/test/java/org/apache/hadoop/hbase/shaded/protobuf/TestProtobufUtil.java b/hbase-client/src/test/java/org/apache/hadoop/hbase/shaded/protobuf/TestProtobufUtil.java index 38e628dc1f2a..e548be4eac22 100644 --- a/hbase-client/src/test/java/org/apache/hadoop/hbase/shaded/protobuf/TestProtobufUtil.java +++ b/hbase-client/src/test/java/org/apache/hadoop/hbase/shaded/protobuf/TestProtobufUtil.java @@ -57,7 +57,6 @@ import org.apache.hbase.thirdparty.com.google.protobuf.ByteString; import org.apache.hbase.thirdparty.com.google.protobuf.BytesValue; import org.apache.hbase.thirdparty.com.google.protobuf.InvalidProtocolBufferException; -import org.apache.hbase.thirdparty.com.google.protobuf.UnsafeByteOperations; import org.apache.hadoop.hbase.shaded.protobuf.generated.CellProtos; import org.apache.hadoop.hbase.shaded.protobuf.generated.ClientProtos; @@ -737,9 +736,9 @@ public void testGetShortTextFormatMutateRequest() throws IOException { @Test public void testGetShortTextFormatCoprocessorServiceRequest() { ClientProtos.CoprocessorServiceCall call = ClientProtos.CoprocessorServiceCall.newBuilder() - .setRow(UnsafeByteOperations.unsafeWrap(Bytes.toBytes("foo"))).setMethodName("awesomeMethod") + .setRow(ByteString.copyFrom(Bytes.toBytes("foo"))).setMethodName("awesomeMethod") .setServiceName("awesomeService") - .setRequest(UnsafeByteOperations.unsafeWrap(Bytes.toBytes("foo-request"))).build(); + .setRequest(ByteString.copyFrom(Bytes.toBytes("foo-request"))).build(); ClientProtos.CoprocessorServiceRequest.Builder builder = ClientProtos.CoprocessorServiceRequest.newBuilder(); From 4755420c8d5caa0adfd6418ae156f1a20fac3e21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1vid=20Paksy?= Date: Thu, 10 Oct 2024 10:48:04 +0200 Subject: [PATCH 5/5] HBASE-28865: Fixed checkstyle issue: too long line in TestProtobufUtil --- .../hadoop/hbase/shaded/protobuf/TestProtobufUtil.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/hbase-client/src/test/java/org/apache/hadoop/hbase/shaded/protobuf/TestProtobufUtil.java b/hbase-client/src/test/java/org/apache/hadoop/hbase/shaded/protobuf/TestProtobufUtil.java index e548be4eac22..02476ad1b961 100644 --- a/hbase-client/src/test/java/org/apache/hadoop/hbase/shaded/protobuf/TestProtobufUtil.java +++ b/hbase-client/src/test/java/org/apache/hadoop/hbase/shaded/protobuf/TestProtobufUtil.java @@ -677,9 +677,8 @@ public void testGetShortTextFormatRegionServerStartupRequest() { String actual = ProtobufUtil.getShortTextFormat(regionServerStartupRequest); assertNotNull(actual); - assertEquals( - "port: 8080 server_start_code: 15 server_current_time: 111111 use_this_hostname_instead: \"some-host-name\"", - actual); + assertEquals("port: 8080 server_start_code: 15 server_current_time: 111111" + + " use_this_hostname_instead: \"some-host-name\"", actual); } @Test