[GitHub] [accumulo] dlmarion commented on a diff in pull request #2647: Consolidated duplicate Thrift client code

[GitBox <gi...@apache.org>]

dlmarion commented on code in PR #2647:
URL: https://github.com/apache/accumulo/pull/2647#discussion_r857536655


##########
core/src/main/java/org/apache/accumulo/core/clientImpl/InstanceOperationsImpl.java:
##########
@@ -120,15 +124,19 @@ private void checkLocalityGroups(String propChanged)
   @Override
   public Map<String,String> getSystemConfiguration()
       throws AccumuloException, AccumuloSecurityException {
-    return ServerClient.execute(context, client -> client.getConfiguration(TraceUtil.traceInfo(),
-        context.rpcCreds(), ConfigurationType.CURRENT));
+    return ThriftClientTypes.CLIENT.executeOnTServer(context, client -> {
+      return client.getConfiguration(TraceUtil.traceInfo(), context.rpcCreds(),
+          ConfigurationType.CURRENT);
+    });

Review Comment:
   It appears that the explicit `return`, `{}`, trailing `;` can be removed when it can determine the return type. For example, the method that you reference above becomes: 
   ```
     @Override
     public Map<String,String> getSystemConfiguration()
         throws AccumuloException, AccumuloSecurityException {
       return ThriftClientTypes.CLIENT.executeOnTServer(context, client ->
         client.getConfiguration(TraceUtil.traceInfo(), context.rpcCreds(),
             ConfigurationType.CURRENT));
     }
   ```
   
   I didn't realize that there was an implicit return. I'll see what I can do to consolidate this even further.
   
   However, there is another method in this class where removing these things does not work as it cannot determine the type for `R` in `ThriftClientType.executeAdminOnManager` :
   ```
     @Override
     public void removeProperty(final String property)
         throws AccumuloException, AccumuloSecurityException {
       checkArgument(property != null, "property is null");
       DeprecatedPropertyUtil.getReplacementName(property, (log, replacement) -> {
         // force a warning on the client side, but send the name the user used to the server-side
         // to trigger a warning in the server logs, and to handle it there
         log.warn("{} was deprecated and will be removed in a future release; assuming user meant"
             + " its replacement {} and will remove that instead", property, replacement);
       });
       ThriftClientTypes.MANAGER.executeAdminOnManager(context, client -> {
         client.removeSystemProperty(TraceUtil.traceInfo(), context.rpcCreds(), property);
         return null;
       });
       checkLocalityGroups(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: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org