keith-turner commented on code in PR #5775:
URL: https://github.com/apache/accumulo/pull/5775#discussion_r2251762482
##########
core/src/main/java/org/apache/accumulo/core/rpc/clients/TServerClient.java:
##########
@@ -60,16 +63,39 @@
public interface TServerClient<C extends TServiceClient> {
static final String DEBUG_HOST = "org.apache.accumulo.client.rpc.debug.host";
+ static final String DEBUG_RG = "org.apache.accumulo.client.rpc.debug.group";
- Pair<String,C> getThriftServerConnection(ClientContext context, boolean
preferCachedConnections)
- throws TTransportException;
+ Pair<String,C> getThriftServerConnection(ClientContext context, boolean
preferCachedConnections,
+ ResourceGroupId rgid) throws TTransportException;
Review Comment:
Seems like this method could take a predicate as an argument instead of a
ResourceGroup. Then would not need to create the ANY ResourceGroupId. It would
be nice to remove ResourceGroupId.ANY because otherwise we need to check for it
in places in the public API where its not expected and throw
IllegalArgumentException.
```suggestion
ResourceGroupPredicate rgp) throws TTransportException;
```
##########
core/src/main/java/org/apache/accumulo/core/rpc/clients/TServerClient.java:
##########
@@ -60,16 +63,39 @@
public interface TServerClient<C extends TServiceClient> {
static final String DEBUG_HOST = "org.apache.accumulo.client.rpc.debug.host";
+ static final String DEBUG_RG = "org.apache.accumulo.client.rpc.debug.group";
- Pair<String,C> getThriftServerConnection(ClientContext context, boolean
preferCachedConnections)
- throws TTransportException;
+ Pair<String,C> getThriftServerConnection(ClientContext context, boolean
preferCachedConnections,
+ ResourceGroupId rgid) throws TTransportException;
default Pair<String,C> getThriftServerConnection(Logger LOG,
ThriftClientTypes<C> type,
ClientContext context, boolean preferCachedConnections, AtomicBoolean
warned,
- ThriftService service) throws TTransportException {
+ ThriftService service, ResourceGroupId rgid) throws TTransportException {
checkArgument(context != null, "context is null");
final String debugHost = System.getProperty(DEBUG_HOST, null);
+ final String debugRG = System.getProperty(DEBUG_RG, null);
+
+ if (debugHost != null && debugRG != null) {
+ LOG.warn("System properties {} and {} are both set. If set incorrectly
then"
+ + " this client may not find a server to connect to.", DEBUG_HOST,
DEBUG_RG);
+ }
+
+ ResourceGroupPredicate rgp = rgid == ResourceGroupId.ANY ? r -> true : r
-> r.equals(rgid);
+ if (debugRG != null && !rgid.canonical().equals(debugRG)) {
+ if (type == ThriftClientTypes.CLIENT || type ==
ThriftClientTypes.COMPACTOR
+ || type == ThriftClientTypes.SERVER_PROCESS || type ==
ThriftClientTypes.TABLET_INGEST
+ || type == ThriftClientTypes.TABLET_MGMT || type ==
ThriftClientTypes.TABLET_SCAN
+ || type == ThriftClientTypes.TABLET_SERVER) {
+ LOG.debug("System property '{}' set to '{}' overriding group argument
'{}'", DEBUG_RG,
+ debugRG, rgid);
+ rgp = r -> r.canonical().equals(debugRG);
+ } else {
+ LOG.debug(
+ "System property '{}' set to '{}' but ignored when making RPCs to
management servers",
+ DEBUG_RG, debugRG);
+ }
+ }
Review Comment:
It does not seem like switching the resource group from what other code
specified is safe. For the planned changes in #5749 changing the RG could lead
to the API call returning incorrect data. So seems like it should warn or error
when debug request conflicts w/ what the code requested since the implications
of changing it are unknown and may cause problems.
The following suggestion is incomplete, was not sure if we should error or
warn and ignore when disjoint.
```suggestion
ResourceGroupPredicate rgp = rgid == ResourceGroupId.ANY ? r -> true : r
-> r.equals(rgid);
if (debugRG != null)
if(rgp.test(debugRG)) {
// its safe to potentially narrow the predicate
rgp = r -> r.canonical().equals(debugRG);
}else{
// the predicate and debugRG are disjoint.
// TODO could ignore the debugRG and warn or error here, not sure
what is best
LOG.warn("System property '{}' set to '{}' was ????");
// TODO not sure the following makes sense or not? Maybe just warn?
rgp = r -> false;
}
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
