HBASE-29289: Make hbase master ui show cluster client connections info#8058
HBASE-29289: Make hbase master ui show cluster client connections info#8058hingu-8103 wants to merge 1 commit into
Conversation
|
|
||
| // Verify that the connection is unregistered | ||
| connections = registry.getClientConnections(); | ||
| assertTrue(connections.size() <= initialSize); |
There was a problem hiding this 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 ?
There was a problem hiding this comment.
I do not think other tests can connect to the rpc server started in this test?
There was a problem hiding this comment.
Thanks @Apache9 for the clarification. I have verified it won't introduce any flakiness.
wchevreuil
left a comment
wchevreuil
left a comment
There was a problem hiding this comment.
I think we should try to reuse the existing metrics subsystem for tracking and reporting these info. Please look at MetricsUserSourceImpl, MetricsUserAggregateImpl, LossyCounting, ClusterStatus.ClientMetrics. If we reuse the existing metric system, we don't need the extra tracking logic at related RPC layer classes and it may also make it easier to update the hbtop views.
There was a problem hiding this 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.
There was a problem hiding this comment.
Thanks @wchevreuil for the comments, Let me go through the existing implementation and try to use that for this purpose.
PDavid
left a comment
PDavid
left a comment
There was a problem hiding this comment.
Many thanks, looks nice already.
| <td><%= clientConnection.getUserName() %></td> | ||
| <td><%= clientConnection.getClientVersion() %></td> | ||
| <td><%= clientConnection.getServiceName() %></td> | ||
| <td><%= serverName.getServerName() %></td>cc |
There was a problem hiding this comment.
Here there's a stray cc after the closing tag that will render as visible text in the HTML.
| <td><%= serverName.getServerName() %></td>cc | |
| <td><%= serverName.getServerName() %></td> |
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; |
There was a problem hiding this comment.
These are unused imports. Can we remove them?
| Optional<ServerRpcConnection> rpcConnectionOptional = RpcServer.getCurrentServerRpcConnection(); | ||
| if (rpcConnectionOptional.isPresent()) { | ||
| ServerRpcConnection rpcConnection = rpcConnectionOptional.get(); | ||
| hostAddress = rpcConnection.getLocalHostAddress(); |
There was a problem hiding this 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?
| <td><%= clientConnection.getUserName() %></td> | ||
| <td><%= clientConnection.getClientVersion() %></td> | ||
| <td><%= clientConnection.getServiceName() %></td> |
There was a problem hiding this 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.
| optional string host_address = 5; | ||
| optional string user_name = 6; | ||
| optional string client_version = 7; | ||
| optional string service_name = 8; |
There was a problem hiding this comment.
Other fields are all having comments. Would it make sense to add comments for these new fields as well?
4 participants
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.