You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by "hingu-8103 (via GitHub)" <gi...@apache.org> on 2026/04/10 10:46:00 UTC
[PR] HBASE-29289: Make hbase master ui show cluster client connections info [hbase]
hingu-8103 opened a new pull request, #8058:
URL: https://github.com/apache/hbase/pull/8058
Added ClientConnectionInfo object which tracks client connection info for each client connections and also added ClientConnectionRegistry to track all the active ClientConnectionInfo.
I have added basic tests to test the change and also manually tested on the cluster.
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29289: Make hbase master ui show cluster client connections info [hbase]
Posted by "wchevreuil (via GitHub)" <gi...@apache.org>.
wchevreuil commented on PR #8058:
URL: https://github.com/apache/hbase/pull/8058#issuecomment-4718151822
> @wchevreuil sorry about that i forgot mention about that property, Do we need to document this anywhere ? cc : @PDavid
Just add it on the jira release-notes field.
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29289: Make hbase master ui show cluster client connections info [hbase]
Posted by "hingu-8103 (via GitHub)" <gi...@apache.org>.
hingu-8103 commented on code in PR #8058:
URL: https://github.com/apache/hbase/pull/8058#discussion_r3063749727
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestNettyServerRpcConnectionClientConnectionInfo.java:
##########
@@ -0,0 +1,195 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.ipc;
+
+import static org.apache.hadoop.hbase.ipc.TestProtobufRpcServiceImpl.SERVICE;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.util.Collection;
+import java.util.concurrent.TimeUnit;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.HBaseConfiguration;
+import org.apache.hadoop.hbase.Waiter;
+import org.apache.hadoop.hbase.testclassification.MediumTests;
+import org.apache.hadoop.hbase.testclassification.RPCTests;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import org.apache.hbase.thirdparty.com.google.common.collect.Lists;
+import org.apache.hbase.thirdparty.com.google.common.io.Closeables;
+import org.apache.hbase.thirdparty.io.netty.channel.Channel;
+import org.apache.hbase.thirdparty.io.netty.channel.nio.NioEventLoopGroup;
+import org.apache.hbase.thirdparty.io.netty.channel.socket.nio.NioSocketChannel;
+
+import org.apache.hadoop.hbase.shaded.ipc.protobuf.generated.TestProtos.EchoRequestProto;
+import org.apache.hadoop.hbase.shaded.ipc.protobuf.generated.TestRpcServiceProtos.TestProtobufRpcProto;
+
+/**
+ * Test to verify that ClientConnectionInfo is properly registered and unregistered in
+ * NettyServerRpcConnection lifecycle to prevent memory leaks.
+ */
+@Category({ RPCTests.class, MediumTests.class })
+public class TestNettyServerRpcConnectionClientConnectionInfo {
+
+ @ClassRule
+ public static final HBaseClassTestRule CLASS_RULE =
+ HBaseClassTestRule.forClass(TestNettyServerRpcConnectionClientConnectionInfo.class);
+
+ private static Configuration CONF = HBaseConfiguration.create();
+ private NioEventLoopGroup group;
+ private NettyRpcServer server;
+ private NettyRpcClient client;
+ private TestProtobufRpcProto.BlockingInterface stub;
+
+ @Before
+ public void setUp() throws IOException {
+ group = new NioEventLoopGroup();
+ server = new NettyRpcServer(null, getClass().getSimpleName(),
+ Lists.newArrayList(new RpcServer.BlockingServiceAndInterface(SERVICE, null)),
+ new InetSocketAddress("localhost", 0), CONF, new FifoRpcScheduler(CONF, 1), true);
+ NettyRpcClientConfigHelper.setEventLoopConfig(CONF, group, NioSocketChannel.class);
+ client = new NettyRpcClient(CONF);
+ server.start();
+ stub = TestProtobufRpcServiceImpl.newBlockingStub(client, server.getListenerAddress());
+ }
+
+ @After
+ public void tearDown() throws Exception {
+ Closeables.close(client, true);
+ server.stop();
+ group.shutdownGracefully().sync();
+ }
+
+ @Test
+ public void testClientConnectionInfoRegisteredAndUnregisteredFromRegistry() throws Exception {
+ ClientConnectionRegistry registry = server.getClientConnectionRegistry();
+ // Verify that the registry is not null
+ assertNotNull(registry);
+
+ // Get the initial number of client connections registered
+ Collection<ClientConnectionInfo> connections = registry.getClientConnections();
+ int initialSize = connections.size();
+
+ // Make an RPC call which will trigger connection header read and registration
+ assertEquals("test",
+ stub.echo(null, EchoRequestProto.newBuilder().setMessage("test").build()).getMessage());
+
+ // Wait for the ClientConnectionInfo to be registered
+ Waiter.waitFor(CONF, 5000, () -> registry.getClientConnections().size() > initialSize);
+
+ // Verify that one connection is registered
+ connections = registry.getClientConnections();
+ assertTrue(connections.size() >= initialSize + 1);
+
+ // Get the channel to verify it closes
+ Channel channel = server.allChannels.stream().filter(c -> c instanceof NioSocketChannel)
+ .findFirst().orElse(null);
+ assertNotNull(channel);
+
+ // Close the client connection
+ client.close();
+
+ // Wait for the channel to close
+ channel.closeFuture().await(5000, TimeUnit.MILLISECONDS);
+
+ // Wait for the ClientConnectionInfo to be unregistered
+ Waiter.waitFor(CONF, 5000, () -> registry.getClientConnections().size() <= initialSize);
+
+ // Verify that the connection is unregistered
+ connections = registry.getClientConnections();
+ assertTrue(connections.size() <= initialSize);
Review Comment:
I believe this assertion might introduce flakiness in tests when there are new client connections happens during this test execution by some other test case which is running parallel.
How should i make test robust in this kind of scenarios.
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29289: Make hbase master ui show cluster client connections info [hbase]
Posted by "hingu-8103 (via GitHub)" <gi...@apache.org>.
hingu-8103 commented on PR #8058:
URL: https://github.com/apache/hbase/pull/8058#issuecomment-4669009996
I've addressed all the comments and added tests as well for this task.
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29289: Make hbase master ui show cluster client connections info [hbase]
Posted by "hingu-8103 (via GitHub)" <gi...@apache.org>.
hingu-8103 commented on code in PR #8058:
URL: https://github.com/apache/hbase/pull/8058#discussion_r3063749727
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestNettyServerRpcConnectionClientConnectionInfo.java:
##########
@@ -0,0 +1,195 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.ipc;
+
+import static org.apache.hadoop.hbase.ipc.TestProtobufRpcServiceImpl.SERVICE;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.util.Collection;
+import java.util.concurrent.TimeUnit;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.HBaseConfiguration;
+import org.apache.hadoop.hbase.Waiter;
+import org.apache.hadoop.hbase.testclassification.MediumTests;
+import org.apache.hadoop.hbase.testclassification.RPCTests;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import org.apache.hbase.thirdparty.com.google.common.collect.Lists;
+import org.apache.hbase.thirdparty.com.google.common.io.Closeables;
+import org.apache.hbase.thirdparty.io.netty.channel.Channel;
+import org.apache.hbase.thirdparty.io.netty.channel.nio.NioEventLoopGroup;
+import org.apache.hbase.thirdparty.io.netty.channel.socket.nio.NioSocketChannel;
+
+import org.apache.hadoop.hbase.shaded.ipc.protobuf.generated.TestProtos.EchoRequestProto;
+import org.apache.hadoop.hbase.shaded.ipc.protobuf.generated.TestRpcServiceProtos.TestProtobufRpcProto;
+
+/**
+ * Test to verify that ClientConnectionInfo is properly registered and unregistered in
+ * NettyServerRpcConnection lifecycle to prevent memory leaks.
+ */
+@Category({ RPCTests.class, MediumTests.class })
+public class TestNettyServerRpcConnectionClientConnectionInfo {
+
+ @ClassRule
+ public static final HBaseClassTestRule CLASS_RULE =
+ HBaseClassTestRule.forClass(TestNettyServerRpcConnectionClientConnectionInfo.class);
+
+ private static Configuration CONF = HBaseConfiguration.create();
+ private NioEventLoopGroup group;
+ private NettyRpcServer server;
+ private NettyRpcClient client;
+ private TestProtobufRpcProto.BlockingInterface stub;
+
+ @Before
+ public void setUp() throws IOException {
+ group = new NioEventLoopGroup();
+ server = new NettyRpcServer(null, getClass().getSimpleName(),
+ Lists.newArrayList(new RpcServer.BlockingServiceAndInterface(SERVICE, null)),
+ new InetSocketAddress("localhost", 0), CONF, new FifoRpcScheduler(CONF, 1), true);
+ NettyRpcClientConfigHelper.setEventLoopConfig(CONF, group, NioSocketChannel.class);
+ client = new NettyRpcClient(CONF);
+ server.start();
+ stub = TestProtobufRpcServiceImpl.newBlockingStub(client, server.getListenerAddress());
+ }
+
+ @After
+ public void tearDown() throws Exception {
+ Closeables.close(client, true);
+ server.stop();
+ group.shutdownGracefully().sync();
+ }
+
+ @Test
+ public void testClientConnectionInfoRegisteredAndUnregisteredFromRegistry() throws Exception {
+ ClientConnectionRegistry registry = server.getClientConnectionRegistry();
+ // Verify that the registry is not null
+ assertNotNull(registry);
+
+ // Get the initial number of client connections registered
+ Collection<ClientConnectionInfo> connections = registry.getClientConnections();
+ int initialSize = connections.size();
+
+ // Make an RPC call which will trigger connection header read and registration
+ assertEquals("test",
+ stub.echo(null, EchoRequestProto.newBuilder().setMessage("test").build()).getMessage());
+
+ // Wait for the ClientConnectionInfo to be registered
+ Waiter.waitFor(CONF, 5000, () -> registry.getClientConnections().size() > initialSize);
+
+ // Verify that one connection is registered
+ connections = registry.getClientConnections();
+ assertTrue(connections.size() >= initialSize + 1);
+
+ // Get the channel to verify it closes
+ Channel channel = server.allChannels.stream().filter(c -> c instanceof NioSocketChannel)
+ .findFirst().orElse(null);
+ assertNotNull(channel);
+
+ // Close the client connection
+ client.close();
+
+ // Wait for the channel to close
+ channel.closeFuture().await(5000, TimeUnit.MILLISECONDS);
+
+ // Wait for the ClientConnectionInfo to be unregistered
+ Waiter.waitFor(CONF, 5000, () -> registry.getClientConnections().size() <= initialSize);
+
+ // Verify that the connection is unregistered
+ connections = registry.getClientConnections();
+ assertTrue(connections.size() <= initialSize);
Review Comment:
I believe this assertion might introduce flakiness in tests when there are new client connections happens during this test execution by some other test case which is running parallel.
How should i make test robust in this kind of scenarios ?
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29289: Make hbase master ui show cluster client connections info [hbase]
Posted by "wchevreuil (via GitHub)" <gi...@apache.org>.
wchevreuil commented on PR #8058:
URL: https://github.com/apache/hbase/pull/8058#issuecomment-4709032863
How should I test it? I'm trying by running a full scan on a table from hbase shell, however, I don't see any updates for th "client connections" tab in Master UI, not the related view in hbtop. Am I missing something here?
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29289: Make hbase master ui show cluster client connections info [hbase]
Posted by "Apache9 (via GitHub)" <gi...@apache.org>.
Apache9 commented on code in PR #8058:
URL: https://github.com/apache/hbase/pull/8058#discussion_r3068219849
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestNettyServerRpcConnectionClientConnectionInfo.java:
##########
@@ -0,0 +1,195 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.ipc;
+
+import static org.apache.hadoop.hbase.ipc.TestProtobufRpcServiceImpl.SERVICE;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.util.Collection;
+import java.util.concurrent.TimeUnit;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.HBaseConfiguration;
+import org.apache.hadoop.hbase.Waiter;
+import org.apache.hadoop.hbase.testclassification.MediumTests;
+import org.apache.hadoop.hbase.testclassification.RPCTests;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import org.apache.hbase.thirdparty.com.google.common.collect.Lists;
+import org.apache.hbase.thirdparty.com.google.common.io.Closeables;
+import org.apache.hbase.thirdparty.io.netty.channel.Channel;
+import org.apache.hbase.thirdparty.io.netty.channel.nio.NioEventLoopGroup;
+import org.apache.hbase.thirdparty.io.netty.channel.socket.nio.NioSocketChannel;
+
+import org.apache.hadoop.hbase.shaded.ipc.protobuf.generated.TestProtos.EchoRequestProto;
+import org.apache.hadoop.hbase.shaded.ipc.protobuf.generated.TestRpcServiceProtos.TestProtobufRpcProto;
+
+/**
+ * Test to verify that ClientConnectionInfo is properly registered and unregistered in
+ * NettyServerRpcConnection lifecycle to prevent memory leaks.
+ */
+@Category({ RPCTests.class, MediumTests.class })
+public class TestNettyServerRpcConnectionClientConnectionInfo {
+
+ @ClassRule
+ public static final HBaseClassTestRule CLASS_RULE =
+ HBaseClassTestRule.forClass(TestNettyServerRpcConnectionClientConnectionInfo.class);
+
+ private static Configuration CONF = HBaseConfiguration.create();
+ private NioEventLoopGroup group;
+ private NettyRpcServer server;
+ private NettyRpcClient client;
+ private TestProtobufRpcProto.BlockingInterface stub;
+
+ @Before
+ public void setUp() throws IOException {
+ group = new NioEventLoopGroup();
+ server = new NettyRpcServer(null, getClass().getSimpleName(),
+ Lists.newArrayList(new RpcServer.BlockingServiceAndInterface(SERVICE, null)),
+ new InetSocketAddress("localhost", 0), CONF, new FifoRpcScheduler(CONF, 1), true);
+ NettyRpcClientConfigHelper.setEventLoopConfig(CONF, group, NioSocketChannel.class);
+ client = new NettyRpcClient(CONF);
+ server.start();
+ stub = TestProtobufRpcServiceImpl.newBlockingStub(client, server.getListenerAddress());
+ }
+
+ @After
+ public void tearDown() throws Exception {
+ Closeables.close(client, true);
+ server.stop();
+ group.shutdownGracefully().sync();
+ }
+
+ @Test
+ public void testClientConnectionInfoRegisteredAndUnregisteredFromRegistry() throws Exception {
+ ClientConnectionRegistry registry = server.getClientConnectionRegistry();
+ // Verify that the registry is not null
+ assertNotNull(registry);
+
+ // Get the initial number of client connections registered
+ Collection<ClientConnectionInfo> connections = registry.getClientConnections();
+ int initialSize = connections.size();
+
+ // Make an RPC call which will trigger connection header read and registration
+ assertEquals("test",
+ stub.echo(null, EchoRequestProto.newBuilder().setMessage("test").build()).getMessage());
+
+ // Wait for the ClientConnectionInfo to be registered
+ Waiter.waitFor(CONF, 5000, () -> registry.getClientConnections().size() > initialSize);
+
+ // Verify that one connection is registered
+ connections = registry.getClientConnections();
+ assertTrue(connections.size() >= initialSize + 1);
+
+ // Get the channel to verify it closes
+ Channel channel = server.allChannels.stream().filter(c -> c instanceof NioSocketChannel)
+ .findFirst().orElse(null);
+ assertNotNull(channel);
+
+ // Close the client connection
+ client.close();
+
+ // Wait for the channel to close
+ channel.closeFuture().await(5000, TimeUnit.MILLISECONDS);
+
+ // Wait for the ClientConnectionInfo to be unregistered
+ Waiter.waitFor(CONF, 5000, () -> registry.getClientConnections().size() <= initialSize);
+
+ // Verify that the connection is unregistered
+ connections = registry.getClientConnections();
+ assertTrue(connections.size() <= initialSize);
Review Comment:
I do not think other tests can connect to the rpc server started in this test?
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29289: Make hbase master ui show cluster client connections info [hbase]
Posted by "hingu-8103 (via GitHub)" <gi...@apache.org>.
hingu-8103 commented on PR #8058:
URL: https://github.com/apache/hbase/pull/8058#issuecomment-4677717751
The PR run failed due to flakey tests. I ran flakey tests locally on this branch and they all passed.
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29289: Make hbase master ui show cluster client connections info [hbase]
Posted by "hingu-8103 (via GitHub)" <gi...@apache.org>.
hingu-8103 commented on code in PR #8058:
URL: https://github.com/apache/hbase/pull/8058#discussion_r3387375347
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/UserMetricsBuilder.java:
##########
@@ -79,13 +84,27 @@ public static class ClientMetricsImpl implements UserMetrics.ClientMetrics {
private final String hostName;
private final long readRequestCount;
private final long writeRequestCount;
+ private final String hostAddress;
Review Comment:
I'm trying to store IP address of the host in hostAddress whereas hostName can have both Name/IP.
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29289: Make hbase master ui show cluster client connections info [hbase]
Posted by "wchevreuil (via GitHub)" <gi...@apache.org>.
wchevreuil merged PR #8058:
URL: https://github.com/apache/hbase/pull/8058
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29289: Make hbase master ui show cluster client connections info [hbase]
Posted by "hingu-8103 (via GitHub)" <gi...@apache.org>.
hingu-8103 commented on PR #8058:
URL: https://github.com/apache/hbase/pull/8058#issuecomment-4709790918
@wchevreuil sorry about that i forgot mention about that property, Do we need to document this anywhere ?
cc : @PDavid
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29289: Make hbase master ui show cluster client connections info [hbase]
Posted by "wchevreuil (via GitHub)" <gi...@apache.org>.
wchevreuil commented on code in PR #8058:
URL: https://github.com/apache/hbase/pull/8058#discussion_r3072513409
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ClientConnectionRegistry.java:
##########
Review Comment:
Another reason to try integrate this into the already existing client metrics: MetricsUserAggregateImpl already provides its own maps and tracking mechanisms, which would dismiss the need for changes in the rpc layer.
##########
hbase-protocol-shaded/src/main/protobuf/server/ClusterStatus.proto:
##########
Review Comment:
There's already a ClientMetrics object defined in ClusterStatus.proto, which is used by the hbtop tool to provide some client related information. I think we should add these there and reuse that object on this UI, while also update hbtop to report the additional information.
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29289: Make hbase master ui show cluster client connections info [hbase]
Posted by "hingu-8103 (via GitHub)" <gi...@apache.org>.
hingu-8103 commented on code in PR #8058:
URL: https://github.com/apache/hbase/pull/8058#discussion_r3069129883
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestNettyServerRpcConnectionClientConnectionInfo.java:
##########
@@ -0,0 +1,195 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.ipc;
+
+import static org.apache.hadoop.hbase.ipc.TestProtobufRpcServiceImpl.SERVICE;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.util.Collection;
+import java.util.concurrent.TimeUnit;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.HBaseConfiguration;
+import org.apache.hadoop.hbase.Waiter;
+import org.apache.hadoop.hbase.testclassification.MediumTests;
+import org.apache.hadoop.hbase.testclassification.RPCTests;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import org.apache.hbase.thirdparty.com.google.common.collect.Lists;
+import org.apache.hbase.thirdparty.com.google.common.io.Closeables;
+import org.apache.hbase.thirdparty.io.netty.channel.Channel;
+import org.apache.hbase.thirdparty.io.netty.channel.nio.NioEventLoopGroup;
+import org.apache.hbase.thirdparty.io.netty.channel.socket.nio.NioSocketChannel;
+
+import org.apache.hadoop.hbase.shaded.ipc.protobuf.generated.TestProtos.EchoRequestProto;
+import org.apache.hadoop.hbase.shaded.ipc.protobuf.generated.TestRpcServiceProtos.TestProtobufRpcProto;
+
+/**
+ * Test to verify that ClientConnectionInfo is properly registered and unregistered in
+ * NettyServerRpcConnection lifecycle to prevent memory leaks.
+ */
+@Category({ RPCTests.class, MediumTests.class })
+public class TestNettyServerRpcConnectionClientConnectionInfo {
+
+ @ClassRule
+ public static final HBaseClassTestRule CLASS_RULE =
+ HBaseClassTestRule.forClass(TestNettyServerRpcConnectionClientConnectionInfo.class);
+
+ private static Configuration CONF = HBaseConfiguration.create();
+ private NioEventLoopGroup group;
+ private NettyRpcServer server;
+ private NettyRpcClient client;
+ private TestProtobufRpcProto.BlockingInterface stub;
+
+ @Before
+ public void setUp() throws IOException {
+ group = new NioEventLoopGroup();
+ server = new NettyRpcServer(null, getClass().getSimpleName(),
+ Lists.newArrayList(new RpcServer.BlockingServiceAndInterface(SERVICE, null)),
+ new InetSocketAddress("localhost", 0), CONF, new FifoRpcScheduler(CONF, 1), true);
+ NettyRpcClientConfigHelper.setEventLoopConfig(CONF, group, NioSocketChannel.class);
+ client = new NettyRpcClient(CONF);
+ server.start();
+ stub = TestProtobufRpcServiceImpl.newBlockingStub(client, server.getListenerAddress());
+ }
+
+ @After
+ public void tearDown() throws Exception {
+ Closeables.close(client, true);
+ server.stop();
+ group.shutdownGracefully().sync();
+ }
+
+ @Test
+ public void testClientConnectionInfoRegisteredAndUnregisteredFromRegistry() throws Exception {
+ ClientConnectionRegistry registry = server.getClientConnectionRegistry();
+ // Verify that the registry is not null
+ assertNotNull(registry);
+
+ // Get the initial number of client connections registered
+ Collection<ClientConnectionInfo> connections = registry.getClientConnections();
+ int initialSize = connections.size();
+
+ // Make an RPC call which will trigger connection header read and registration
+ assertEquals("test",
+ stub.echo(null, EchoRequestProto.newBuilder().setMessage("test").build()).getMessage());
+
+ // Wait for the ClientConnectionInfo to be registered
+ Waiter.waitFor(CONF, 5000, () -> registry.getClientConnections().size() > initialSize);
+
+ // Verify that one connection is registered
+ connections = registry.getClientConnections();
+ assertTrue(connections.size() >= initialSize + 1);
+
+ // Get the channel to verify it closes
+ Channel channel = server.allChannels.stream().filter(c -> c instanceof NioSocketChannel)
+ .findFirst().orElse(null);
+ assertNotNull(channel);
+
+ // Close the client connection
+ client.close();
+
+ // Wait for the channel to close
+ channel.closeFuture().await(5000, TimeUnit.MILLISECONDS);
+
+ // Wait for the ClientConnectionInfo to be unregistered
+ Waiter.waitFor(CONF, 5000, () -> registry.getClientConnections().size() <= initialSize);
+
+ // Verify that the connection is unregistered
+ connections = registry.getClientConnections();
+ assertTrue(connections.size() <= initialSize);
Review Comment:
Thanks @Apache9 for the clarification. I have verified it won't introduce any flakiness.
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29289: Make hbase master ui show cluster client connections info [hbase]
Posted by "wchevreuil (via GitHub)" <gi...@apache.org>.
wchevreuil commented on code in PR #8058:
URL: https://github.com/apache/hbase/pull/8058#discussion_r3318351018
##########
hbase-server/src/main/resources/hbase-webapps/master/regionServerList.jsp:
##########
@@ -63,6 +63,9 @@
<li class="nav-item">
<a class="nav-link" href="#tab_replicationStats" data-bs-toggle="tab" role="tab">Replications</a>
</li>
+ <li class="nav-item">
+ <a class="nav-link" href="#tab_clientConnectionsStats" data-bs-toggle="tab" role="tab">ClientConnections</a>
+ </li>
Review Comment:
nit: Client Connections
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29289: Make hbase master ui show cluster client connections info [hbase]
Posted by "hingu-8103 (via GitHub)" <gi...@apache.org>.
hingu-8103 commented on code in PR #8058:
URL: https://github.com/apache/hbase/pull/8058#discussion_r3072583296
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ClientConnectionRegistry.java:
##########
Review Comment:
Thanks @wchevreuil for the comments, Let me go through the existing implementation and try to use that for this purpose.
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29289: Make hbase master ui show cluster client connections info [hbase]
Posted by "PDavid (via GitHub)" <gi...@apache.org>.
PDavid commented on code in PR #8058:
URL: https://github.com/apache/hbase/pull/8058#discussion_r3257813273
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsUserAggregateImpl.java:
##########
@@ -193,4 +204,51 @@ private MetricsUserSource getOrCreateMetricsUser(String user) {
userMetricLossyCounting.add(userSource);
return userSource;
}
+
+ private ClientConnectionContext getClientConnectionContext() {
+ String hostAddress = null;
+ String userName = "Unknown";
+ String clientVersion = "Unknown";
+ String serviceName = "Unknown";
+
+ Optional<ServerRpcConnection> rpcConnectionOptional = RpcServer.getCurrentServerRpcConnection();
+ if (rpcConnectionOptional.isPresent()) {
+ ServerRpcConnection rpcConnection = rpcConnectionOptional.get();
+ hostAddress = rpcConnection.getLocalHostAddress();
Review Comment:
The field `localHostAddress` in `ServerRpcConnection` represents the server-side IP (the RS's own address), but it's surfaced in the UI under the column "ClientIP" (via `getHostAddress()`).
This is confusing, the value shown as "ClientIP" is actually the region server's local bind address, not the client's IP. Shouldn't we use `hostAddress` from `ServerRpcConnection` here instead?
##########
hbase-server/src/main/resources/hbase-webapps/master/regionServerListClientConnectionsStats.jsp:
##########
@@ -0,0 +1,54 @@
+<%@ page contentType="text/html;charset=UTF-8"
+ import="org.apache.hadoop.hbase.ServerName"
+ import="org.apache.hadoop.hbase.master.HMaster"
+ import="org.apache.hadoop.hbase.ServerMetrics"
+ import="org.apache.hadoop.hbase.master.ServerManager"
+ import="org.apache.hadoop.hbase.UserMetrics"
+ import="java.util.Map" %>
+
+<%
+ HMaster master = (HMaster) getServletContext().getAttribute(HMaster.MASTER);
+ ServerName[] serverNames = (ServerName[]) request.getAttribute("serverNames");
+ ServerManager serverManager = master.getServerManager();
+%>
+
+<table id="clientConnectionsStatsTable" class="tablesorter table table-striped">
+ <thead>
+ <tr>
+ <th class="cls_separator">ClientIP</th>
+ <th class="cls_separator">UserName</th>
+ <th class="cls_separator">ClientVersion</th>
+ <th class="cls_separator">ServiceName</th>
+ <th class="cls_separator">ServerInfo</th>
+ </tr>
+ </thead>
+ <tbody>
+ <%
+ for (ServerName serverName: serverNames) {
+ ServerMetrics serverMetrics = serverManager.getLoad(serverName);
+ if(serverMetrics != null) {
+
+ Map<byte[], UserMetrics> userMetricsMap = serverMetrics.getUserMetrics();
+ for(Map.Entry<byte[], UserMetrics> entry : userMetricsMap.entrySet()) {
+ UserMetrics userMetrics = entry.getValue();
+ Map<String, UserMetrics.ClientMetrics> clientMetricsMap = userMetrics.getClientMetrics();
+
+ for(Map.Entry<String, UserMetrics.ClientMetrics> clientEntry : clientMetricsMap.entrySet()) {
+ UserMetrics.ClientMetrics clientConnection = clientEntry.getValue();
+
+ %>
+ <tr>
+ <td><%= clientConnection.getHostAddress() %></td>
+ <td><%= clientConnection.getUserName() %></td>
+ <td><%= clientConnection.getClientVersion() %></td>
+ <td><%= clientConnection.getServiceName() %></td>
+ <td><%= serverName.getServerName() %></td>cc
Review Comment:
Here there's a stray `cc` after the closing </td> tag that will render as visible text in the HTML.
```suggestion
<td><%= serverName.getServerName() %></td>
```
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsUserAggregateImpl.java:
##########
@@ -23,10 +23,15 @@
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.hbase.CompatibilitySingletonFactory;
import org.apache.hadoop.hbase.ipc.RpcServer;
+import org.apache.hadoop.hbase.ipc.ServerRpcConnection;
import org.apache.hadoop.hbase.security.User;
import org.apache.hadoop.hbase.security.UserProvider;
import org.apache.hadoop.hbase.util.LossyCounting;
import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
Review Comment:
These are unused imports. Can we remove them?
##########
hbase-server/src/main/resources/hbase-webapps/master/regionServerListClientConnectionsStats.jsp:
##########
@@ -0,0 +1,54 @@
+<%@ page contentType="text/html;charset=UTF-8"
+ import="org.apache.hadoop.hbase.ServerName"
+ import="org.apache.hadoop.hbase.master.HMaster"
+ import="org.apache.hadoop.hbase.ServerMetrics"
+ import="org.apache.hadoop.hbase.master.ServerManager"
+ import="org.apache.hadoop.hbase.UserMetrics"
+ import="java.util.Map" %>
+
+<%
+ HMaster master = (HMaster) getServletContext().getAttribute(HMaster.MASTER);
+ ServerName[] serverNames = (ServerName[]) request.getAttribute("serverNames");
+ ServerManager serverManager = master.getServerManager();
+%>
+
+<table id="clientConnectionsStatsTable" class="tablesorter table table-striped">
+ <thead>
+ <tr>
+ <th class="cls_separator">ClientIP</th>
+ <th class="cls_separator">UserName</th>
+ <th class="cls_separator">ClientVersion</th>
+ <th class="cls_separator">ServiceName</th>
+ <th class="cls_separator">ServerInfo</th>
+ </tr>
+ </thead>
+ <tbody>
+ <%
+ for (ServerName serverName: serverNames) {
+ ServerMetrics serverMetrics = serverManager.getLoad(serverName);
+ if(serverMetrics != null) {
+
+ Map<byte[], UserMetrics> userMetricsMap = serverMetrics.getUserMetrics();
+ for(Map.Entry<byte[], UserMetrics> entry : userMetricsMap.entrySet()) {
+ UserMetrics userMetrics = entry.getValue();
+ Map<String, UserMetrics.ClientMetrics> clientMetricsMap = userMetrics.getClientMetrics();
+
+ for(Map.Entry<String, UserMetrics.ClientMetrics> clientEntry : clientMetricsMap.entrySet()) {
+ UserMetrics.ClientMetrics clientConnection = clientEntry.getValue();
+
+ %>
+ <tr>
+ <td><%= clientConnection.getHostAddress() %></td>
+ <td><%= clientConnection.getUserName() %></td>
+ <td><%= clientConnection.getClientVersion() %></td>
+ <td><%= clientConnection.getServiceName() %></td>
Review Comment:
Here we directly interpolate values from these client-controlled fields (`userName`, `clientVersion`, `serviceName`) using `<%= ... %>` without HTML escaping. Malicious or buggy clients could inject `<script>` tags. Other HBase JSPs use `StringEscapeUtils.escapeHtml4()` - the same should be applied here.
##########
hbase-protocol-shaded/src/main/protobuf/server/ClusterStatus.proto:
##########
@@ -212,6 +206,11 @@ message ClientMetrics {
/** the current total filtered requests made from a client */
optional uint64 filtered_requests_count = 4;
+
+ optional string host_address = 5;
+ optional string user_name = 6;
+ optional string client_version = 7;
+ optional string service_name = 8;
Review Comment:
Other fields are all having comments. Would it make sense to add comments for these new fields as well?
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29289: Make hbase master ui show cluster client connections info [hbase]
Posted by "wchevreuil (via GitHub)" <gi...@apache.org>.
wchevreuil commented on code in PR #8058:
URL: https://github.com/apache/hbase/pull/8058#discussion_r3316920836
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/UserMetricsBuilder.java:
##########
@@ -79,13 +84,27 @@ public static class ClientMetricsImpl implements UserMetrics.ClientMetrics {
private final String hostName;
private final long readRequestCount;
private final long writeRequestCount;
+ private final String hostAddress;
Review Comment:
Is hostAddress different from hostName or could we just reuse the already existing hostName?
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29289: Make hbase master ui show cluster client connections info [hbase]
Posted by "PDavid (via GitHub)" <gi...@apache.org>.
PDavid commented on PR #8058:
URL: https://github.com/apache/hbase/pull/8058#issuecomment-4709203367
> How should I test it? I'm trying by running a full scan on a table from hbase shell, however, I don't see any updates for th "client connections" tab in Master UI, not the related view in hbtop. Am I missing something here?
@wchevreuil I think when I tested it last time, the key was to add the following to hbase-site.xml:
```xml
<property>
<name>hbase.regionserver.user.metrics.enabled</name>
<value>true</value>
</property>
```
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org