[GitHub] [accumulo] ctubbsii commented on a change in pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable

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

ctubbsii commented on a change in pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#discussion_r641832357



##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/Admin.java
##########
@@ -529,7 +529,8 @@ private void printNameSpaceConfiguration(AccumuloClient accumuloClient, String n
     try (BufferedWriter nsWriter = new BufferedWriter(new FileWriter(namespaceScript, UTF_8))) {
       nsWriter.write(createNsFormat.format(new String[] {namespace}));
       TreeMap<String,String> props = new TreeMap<>();
-      for (Entry<String,String> p : accumuloClient.namespaceOperations().getProperties(namespace)) {
+      for (Entry<String,String> p : accumuloClient.namespaceOperations().getPropertiesMap(namespace)
+          .entrySet()) {
         props.put(p.getKey(), p.getValue());
       }

Review comment:
       Many of these can be simplified with `Map.forEach`:
   
   ```suggestion
         accumuloClient.namespaceOperations().getPropertiesMap(namespace).forEach(props::put);
   ```
   
   Basically, if the body of the loop doesn't throw a checked exception, this is possible, and almost always better.

##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java
##########
@@ -617,10 +617,28 @@ void removeProperty(String tableName, String property)
    *         recently changed properties may not be visible immediately.
    * @throws TableNotFoundException
    *           if the table does not exist
+   * @deprecated since 2.1.0; use {@link #getPropertiesMap(String)} instead.
    */
+  @Deprecated(since = "2.1.0")
   Iterable<Entry<String,String>> getProperties(String tableName)
       throws AccumuloException, TableNotFoundException;
 
+  /**
+   * Gets properties of a table. This operation is asynchronous and eventually consistent. It is not
+   * guaranteed that all tablets in a table will return the same values. Within a few seconds
+   * without another change, all tablets in a table should be consistent. The clone table feature
+   * can be used if consistency is required. This new method returns a Map instead of an Iterable.
+   *
+   * @param tableName
+   *          the name of the table
+   * @return all properties visible by this table (system and per-table properties). Note that
+   *         recently changed properties may not be visible immediately.
+   * @throws TableNotFoundException
+   *           if the table does not exist
+   */
+  Map<String,String> getPropertiesMap(String tableName)

Review comment:
       Would the name "getConfiguration" make more sense here? We always talk in terms of "per-table configuration", not "per-table properties".

##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsImpl.java
##########
@@ -219,7 +220,28 @@ public void removeProperty(final String namespace, final String property)
     } catch (Exception e) {
       throw new AccumuloException(e);
     }
+  }
 
+  @Override
+  public Map<String,String> getPropertiesMap(final String namespace)
+      throws AccumuloException, NamespaceNotFoundException {
+    checkArgument(namespace != null, "namespace is null");
+    try {
+      return ServerClient.executeRaw(context, client -> client
+          .getNamespaceConfiguration(TraceUtil.traceInfo(), context.rpcCreds(), namespace));
+    } catch (ThriftTableOperationException e) {
+      switch (e.getType()) {
+        case NAMESPACE_NOTFOUND:
+          throw new NamespaceNotFoundException(e);
+        case OTHER:
+        default:
+          throw new AccumuloException(e.description, e);
+      }
+    } catch (AccumuloException e) {
+      throw e;
+    } catch (Exception e) {
+      throw new AccumuloException(e);
+    }

Review comment:
       Instead of having duplicate code here and in the `getProperties` methods, the `getProperties` methods should be changed to call this method and then call `.entrySet()` on the map, since that's the only difference between these two method implementations anyway.

##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/OfflineIterator.java
##########
@@ -284,7 +284,7 @@ private TabletMetadata getTabletFiles(Range nextRange) {
     // possible race condition here, if table is renamed
     String tableName = Tables.getTableName(context, tableId);
     AccumuloConfiguration acuTableConf =
-        new ConfigurationCopy(context.tableOperations().getProperties(tableName));
+        new ConfigurationCopy(context.tableOperations().getPropertiesMap(tableName));

Review comment:
       Do we still need the old ConfigurationCopy constructor now that these have been changed to use the constructor that uses a Map? If not, I would delete the constructor that takes an Iterable. It's safe to make that change, since it's internal only, and not public API.

##########
File path: test/src/main/java/org/apache/accumulo/test/functional/DurabilityIT.java
##########
@@ -165,10 +165,10 @@ public void testMetaDurability() throws Exception {
     try (AccumuloClient c = Accumulo.newClient().from(getClientProperties()).build()) {
       String tableName = getUniqueNames(1)[0];
       c.instanceOperations().setProperty(Property.TABLE_DURABILITY.getKey(), "none");
-      Map<String,String> props = map(c.tableOperations().getProperties(MetadataTable.NAME));

Review comment:
       Not sure if the map method is still needed after this change. If it isn't needed, it should be deleted.

##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/ShellPluginConfigurationCommand.java
##########
@@ -99,7 +99,8 @@ protected void removePlugin(final CommandLine cl, final Shell shellState, final
       final Shell shellState, final Class<T> clazz, final Property pluginProp) {
     Iterator<Entry<String,String>> props;
     try {
-      props = shellState.getAccumuloClient().tableOperations().getProperties(tableName).iterator();
+      props = shellState.getAccumuloClient().tableOperations().getPropertiesMap(tableName)
+          .entrySet().iterator();
     } catch (AccumuloException | TableNotFoundException e) {

Review comment:
       This code definitely should assign it to a Map type, and instead of iterating over the iterator, it should loop over the entrySet or call Map.forEach in the loop below this assignment.

##########
File path: test/src/main/java/org/apache/accumulo/test/functional/PermissionsIT.java
##########
@@ -862,7 +861,8 @@ private void testArbitraryProperty(AccumuloClient c, String tableName, boolean h
 
       // Loop through properties to make sure the new property is added to the list
       int count = 0;
-      for (Entry<String,String> property : c.tableOperations().getProperties(tableName)) {
+      for (Entry<String,String> property : c.tableOperations().getPropertiesMap(tableName)
+          .entrySet()) {
         if (property.getKey().equals(propertyName) && property.getValue().equals(description1))
           count++;
       }

Review comment:
       This is a place that could be cleaned up quite a bit with a stream:
   
   ```java
   long count = c.tableOperations().getPropertiesMap(tableName).entrySet().stream().filter(e -> e.getKey().equals(propertyName) && e.getValue().equals(description1)).count();
   ```

##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/NamespaceOperations.java
##########
@@ -202,11 +202,32 @@ void removeProperty(String namespace, String property)
    *           if the user does not have permission
    * @throws NamespaceNotFoundException
    *           if the specified namespace doesn't exist
-   * @since 1.6.0
+   * @deprecated since 2.1.0; use {@link #getPropertiesMap(String)} instead.
    */
+  @Deprecated(since = "2.1.0")

Review comment:
       The `@since` tag shouldn't be removed here. I'd prefer the old method be deprecated. There are two ways to think about this:
   
   1. Thinking about reducing churn, based on what exists now; the right question is: "do we need to remove it?"
   2. Thinking about what the API *should* be, in order to have a clean API; the right question: "do we need to have it?"
   
   I tend to think in terms of the clean API, but I understand we need to be careful of churn. However, that's what the deprecation cycle is *for*. So, I'd prefer to deprecate it, because we don't need both. We don't have to remove it any time soon, though. That decision can be made later.

##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java
##########
@@ -146,8 +146,9 @@ public int execute(final String fullCommand, final CommandLine cl, final Shell s
     // Copy options if flag was set
     if (cl.hasOption(createTableOptCopyConfig.getOpt())) {
       if (shellState.getAccumuloClient().tableOperations().exists(tableName)) {
-        final Iterable<Entry<String,String>> configuration = shellState.getAccumuloClient()
-            .tableOperations().getProperties(cl.getOptionValue(createTableOptCopyConfig.getOpt()));
+        final Iterable<Entry<String,String>> configuration =
+            shellState.getAccumuloClient().tableOperations()
+                .getPropertiesMap(cl.getOptionValue(createTableOptCopyConfig.getOpt())).entrySet();

Review comment:
       Instead of calling the new method, only to call entrySet and assign it to an Iterable, many of these would make more sense being assigned to a variable that is of the Map type instead. The assignments to Iterable types are very clunky, and many of them probably only were written that way because that's the type that the old method returned, not because we wanted them to be an Iterable.




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