[GitHub] [accumulo] keith-turner commented on a change in pull request #1995: Auto Generate Accumulo Client in JShell

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

keith-turner commented on a change in pull request #1995:
URL: https://github.com/apache/accumulo/pull/1995#discussion_r607993675



##########
File path: assemble/src/main/scripts/accumulo-client-build.jsh
##########
@@ -0,0 +1,53 @@
+/*
+# 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.
+*/
+  String accumuloProp = 
+      System.getProperty("user.dir").replace("/bin", "/conf/accumulo-client.properties");

Review comment:
       If possible it would be nice to find the client props file on the classpath.  For the following suggestion I copied code from [here](https://github.com/apache/accumulo/blob/aab34a980d7a9ccad63797913b99b331c5cbbaae/core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java#L207-L216) and modified it a bit.
   
   ```suggestion
     URL clientPropsUrl =
             AccumuloClient.class.getClassLoader().getResource("accumulo-client.properties");
         if (clientPropsUrl != null) {
           accumuloProp = clientPropsUrl.getFile();
         }
   ```
   
   Not sure if this workable though.  It partially depends on how the classpath is setup for accumulo jshell.  If that follows how other java processes are launched, then the conf dir would be on the classpath.  Also not sure if classloader stuff works in jshell.  If it does work it would give the user more control over where the client props file is found because they can easily change the classpath in accumulo-env.sh.

##########
File path: assemble/src/main/scripts/accumulo-client-build.jsh
##########
@@ -0,0 +1,53 @@
+/*
+# 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.
+*/
+  String accumuloProp = 
+      System.getProperty("user.dir").replace("/bin", "/conf/accumulo-client.properties");
+  String name; String zk; String principal; String token; 
+  AccumuloClient client = null; Authorizations auth = null;
+  
+  try {
+    // Does the accumulo properties file exists?
+    if (Files.exists(Paths.get(accumuloProp))) {
+      // Build Accumulo Client
+      client = Accumulo.newClient().from(accumuloProp).build();
+      
+      // Test Accumulo Client
+      auth = client.securityOperations().getUserAuthorizations(client.whoami());
+      if (auth != null) {
+        System.out.println("Accumulo Client was Successfully Built!");
+      System.out.println("Use " + "\"client\"" + " to interact with Accumulo \n");
+      }
+    }
+    else
+      System.out.println("accumulo-client.properties not found in conf directory \n");
+        
+  } catch (IllegalArgumentException illegalExcept) {
+    System.out.println("Build Error: "
+        + "Could not build Accumulo client with accumulo-client.properties \n"); 

Review comment:
       I think these error message would be more useful if they included the full path of the client props file that was used.
   
   ```suggestion
           + "Could not build Accumulo client with "+accumuloProp+" \n"); 
   ```

##########
File path: assemble/src/main/scripts/accumulo-client-build.jsh
##########
@@ -0,0 +1,53 @@
+/*
+# 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.
+*/
+  String accumuloProp = 
+      System.getProperty("user.dir").replace("/bin", "/conf/accumulo-client.properties");
+  String name; String zk; String principal; String token; 
+  AccumuloClient client = null; Authorizations auth = null;
+  
+  try {
+    // Does the accumulo properties file exists?
+    if (Files.exists(Paths.get(accumuloProp))) {
+      // Build Accumulo Client
+      client = Accumulo.newClient().from(accumuloProp).build();
+      
+      // Test Accumulo Client
+      auth = client.securityOperations().getUserAuthorizations(client.whoami());
+      if (auth != null) {
+        System.out.println("Accumulo Client was Successfully Built!");
+      System.out.println("Use " + "\"client\"" + " to interact with Accumulo \n");
+      }
+    }
+    else
+      System.out.println("accumulo-client.properties not found in conf directory \n");
+        
+  } catch (IllegalArgumentException illegalExcept) {
+    System.out.println("Build Error: "
+        + "Could not build Accumulo client with accumulo-client.properties \n"); 
+      
+  } catch (AccumuloException accumuloExcept) {
+    System.out.println("Connection Error: "
+        + "Ensure instance.name and instance.zookeeper are properly configured \n");

Review comment:
       ```suggestion
           + "Ensure instance.name and instance.zookeeper are properly configured in "+accumuloProp+"  \n");
   ```




-- 
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.

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