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:
[email protected]
