You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2021/05/27 16:40:47 UTC
[GitHub] [accumulo] foster33 opened a new pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
foster33 opened a new pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122
Fixes #2120
This fix changes the return type of the getProperties function in TableOperations [here](https://github.com/apache/accumulo/blob/1dc72fce2c781dee597c8c11876a3bc6c321c199/core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java#L621) and NamespaceOperations [here](https://github.com/apache/accumulo/blob/4f35e6aff1763d4b19d1e93036260094b47cc589/core/src/main/java/org/apache/accumulo/core/client/admin/NamespaceOperations.java#L207) to a Map instead of an Iterable by creating a new function named getPropertiesMap. All previous references to getProperties is changed to getPropertiesMap and small changes were made in areas to allow for the map to be iterated when needed.
The original getProperties function has been deprecated.
--
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
[GitHub] [accumulo] foster33 commented on a change in pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
foster33 commented on a change in pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#discussion_r644018023
##########
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:
I have not made any changes to this part. I felt as though it might be better for someone with more experience with the use and nature of the ConfigurationCopy constructor to make the decision on this.
--
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
[GitHub] [accumulo] EdColeman commented on pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
EdColeman commented on pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#issuecomment-856030215
It's okay to merge now - if it's ready.
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#discussion_r644991831
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsHelper.java
##########
@@ -217,7 +215,7 @@ public void removeConstraint(String namespace, int number)
public Map<String,Integer> listConstraints(String namespace)
throws AccumuloException, NamespaceNotFoundException, AccumuloSecurityException {
Map<String,Integer> constraints = new TreeMap<>();
- for (Entry<String,String> property : this.getProperties(namespace)) {
+ for (Entry<String,String> property : this.getConfiguration(namespace).entrySet()) {
Review comment:
Since we're not deprecating, these that are longer/less convenient than the existing code can be reverted back to their former `getProperties`. Sorry for the churn in your PR!
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsHelper.java
##########
@@ -72,9 +72,7 @@ public void removeIterator(String namespace, String name, EnumSet<IteratorScope>
if (!exists(namespace))
throw new NamespaceNotFoundException(null, namespace, null);
Map<String,String> copy = new TreeMap<>();
- for (Entry<String,String> property : this.getProperties(namespace)) {
- copy.put(property.getKey(), property.getValue());
- }
+ this.getConfiguration(namespace).forEach(copy::put);
Review comment:
This could be:
```java
Map<String,String> copy = Map.copyOf(this.getConfiguration(namespace));
```
(so long as we aren't making changes to the copy, because this would make an immutable copy)
There are a few such occurrences, if you want to change them.
##########
File path: test/src/main/java/org/apache/accumulo/test/replication/StatusCombinerMacIT.java
##########
@@ -86,11 +86,9 @@ public void testCombinerSetOnMetadata() throws Exception {
assertTrue(scopes.contains(IteratorScope.minc));
assertTrue(scopes.contains(IteratorScope.majc));
- Iterable<Entry<String,String>> propIter = tops.getProperties(MetadataTable.NAME);
+ Map<String,String> config = tops.getConfiguration(MetadataTable.NAME);
HashMap<String,String> properties = new HashMap<>();
- for (Entry<String,String> entry : propIter) {
- properties.put(entry.getKey(), entry.getValue());
- }
+ config.forEach(properties::put);
Review comment:
Another opportunity for `Map.copyOf`
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java
##########
@@ -617,10 +617,30 @@ void removeProperty(String tableName, String property)
* recently changed properties may not be visible immediately.
* @throws TableNotFoundException
* if the table does not exist
+ * @since 1.6.0
+ * @deprecated since 2.1.0; use {@link #getConfiguration(String)} (String)} instead.
*/
+ @Deprecated(since = "2.1.0")
Iterable<Entry<String,String>> getProperties(String tableName)
throws AccumuloException, TableNotFoundException;
Review comment:
If a default method is used in the interface, most, if not all, of the implementing classes and subclasses can simply remove their implementing method:
```suggestion
* @since 1.6.0
*/
default Iterable<Entry<String,String>> getProperties(String tableName)
throws AccumuloException, TableNotFoundException {
return getConfiguration(tableName).entrySet();
}
```
(and the same can be done in NamespaceOperations and its implementing classes and subclasses)
--
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
[GitHub] [accumulo] foster33 commented on a change in pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
foster33 commented on a change in pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#discussion_r645736193
##########
File path: core/src/test/java/org/apache/accumulo/core/clientImpl/TableOperationsHelperTest.java
##########
@@ -144,10 +144,15 @@ public void removeProperty(String tableName, String property) {
@Override
public Iterable<Entry<String,String>> getProperties(String tableName) {
+ return getConfiguration(tableName).entrySet();
+ }
Review comment:
I had deleted this at first but then had put it back because I was unsure. I will fix this.
--
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
[GitHub] [accumulo] milleruntime commented on a change in pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#discussion_r646570300
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java
##########
@@ -609,16 +609,37 @@ void removeProperty(String tableName, String property)
* 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.
+ * can be used if consistency is required. Method calls getConfiguration and then calls
Review comment:
Could add javadoc links to the `getConfiguration()` method here and in `NamespaceOperations`.
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by 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
[GitHub] [accumulo] keith-turner commented on a change in pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#discussion_r643205304
##########
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:
When thinking about the name, may want to consider [the methods in InstanceOperations](https://github.com/apache/accumulo/blob/8a636a3ba91f5dae1d8b09b095178889a7d79c1d/core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java#L69-L77)
--
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
[GitHub] [accumulo] keith-turner commented on a change in pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#discussion_r643201136
##########
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:
My preference would be to not deprecate or remove the method for the following reasons.
- The method is useful for the case of looping through the properties to find props matching a pattern. This use case is not uncommon and it seems like the existing method supports slightly shorter code.
- For table props, it seems like the existing method has been the only way to get table props in the API since Accumulo was released. So for the past decade any code written would have had to use this method. To me, removing the method when it has no problems seems needlessly disruptive to this existing code.
My preference would be a `@see` javadoc tag pointing to the other method instead of deprecating.
--
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
[GitHub] [accumulo] keith-turner commented on a change in pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#discussion_r643218230
##########
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:
> It does feel like a mistake that the API didn't originally return the more flexible object.
Following that line of thought, maybe Map is not flexible enough either. For example [a specific configuration type was created for plugins](https://github.com/apache/accumulo/blob/8a636a3ba91f5dae1d8b09b095178889a7d79c1d/core/src/main/java/org/apache/accumulo/core/client/PluginEnvironment.java#L41) and it supports an operation like `isSet()`.
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#discussion_r644986541
##########
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:
I'm still in favor of deprecating. Some of the objections so far have been against removing, not against deprecating. The bar for deprecating should be much lower. I think API bloat is a harm that should be considered, and I think the SemVer process mitigates against the concerns over the worst effects of disruption. I would like to clean up API bloat over time, to streamline and simplify our API for ease of use in future, as well as accommodate existing users. I think the deprecation process with SemVer helps balance those competing goals, and think that some of the objections would have our API bloat over time. But, I think API evolution should also include the loss of vestigial APIs.
However, as it stands, I'm outvoted here. So, unless others agree with me, that we should slowly phase out the old vestigial API, using deprecation as a start, we should keep both and not deprecate at this time.
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsHelper.java
##########
@@ -217,7 +215,7 @@ public void removeConstraint(String namespace, int number)
public Map<String,Integer> listConstraints(String namespace)
throws AccumuloException, NamespaceNotFoundException, AccumuloSecurityException {
Map<String,Integer> constraints = new TreeMap<>();
- for (Entry<String,String> property : this.getProperties(namespace)) {
+ for (Entry<String,String> property : this.getConfiguration(namespace).entrySet()) {
Review comment:
Since we're not deprecating, these that are longer/less convenient than the existing code can be reverted back to their former `getProperties`. Sorry for the churn in your PR!
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsHelper.java
##########
@@ -72,9 +72,7 @@ public void removeIterator(String namespace, String name, EnumSet<IteratorScope>
if (!exists(namespace))
throw new NamespaceNotFoundException(null, namespace, null);
Map<String,String> copy = new TreeMap<>();
- for (Entry<String,String> property : this.getProperties(namespace)) {
- copy.put(property.getKey(), property.getValue());
- }
+ this.getConfiguration(namespace).forEach(copy::put);
Review comment:
This could be:
```java
Map<String,String> copy = Map.copyOf(this.getConfiguration(namespace));
```
(so long as we aren't making changes to the copy, because this would make an immutable copy)
There are a few such occurrences, if you want to change them.
##########
File path: test/src/main/java/org/apache/accumulo/test/replication/StatusCombinerMacIT.java
##########
@@ -86,11 +86,9 @@ public void testCombinerSetOnMetadata() throws Exception {
assertTrue(scopes.contains(IteratorScope.minc));
assertTrue(scopes.contains(IteratorScope.majc));
- Iterable<Entry<String,String>> propIter = tops.getProperties(MetadataTable.NAME);
+ Map<String,String> config = tops.getConfiguration(MetadataTable.NAME);
HashMap<String,String> properties = new HashMap<>();
- for (Entry<String,String> entry : propIter) {
- properties.put(entry.getKey(), entry.getValue());
- }
+ config.forEach(properties::put);
Review comment:
Another opportunity for `Map.copyOf`
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java
##########
@@ -617,10 +617,30 @@ void removeProperty(String tableName, String property)
* recently changed properties may not be visible immediately.
* @throws TableNotFoundException
* if the table does not exist
+ * @since 1.6.0
+ * @deprecated since 2.1.0; use {@link #getConfiguration(String)} (String)} instead.
*/
+ @Deprecated(since = "2.1.0")
Iterable<Entry<String,String>> getProperties(String tableName)
throws AccumuloException, TableNotFoundException;
Review comment:
If a default method is used in the interface, most, if not all, of the implementing classes and subclasses can simply remove their implementing method:
```suggestion
* @since 1.6.0
*/
default Iterable<Entry<String,String>> getProperties(String tableName)
throws AccumuloException, TableNotFoundException {
return getConfiguration(tableName).entrySet();
}
```
(and the same can be done in NamespaceOperations and its implementing classes and subclasses)
--
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
[GitHub] [accumulo] keith-turner commented on a change in pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#discussion_r645665149
##########
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:
My thoughts on deprecating are not super well defined, but my thinking on deprecation of API is that it should be related to functionality that will be removed or is fundamentally broken. For this particular case the functionality behind the existing API in questions works fine and will not be removed. Also the new API being introduced is functionally equivalent to the existing API (each could be fully implemented using only the other). We could introduce an infinite number of functionally equivalent APIs and deprecate existing APIs.
Unrelated to deprecation, I also tend to think that new APIs should normally be motivated by new functionality. However its not that well defined for me, because I can think of exceptions to this guide. In this case there is no new functionality, but the new API sill makes sense to me for other reasons. Thinking about this a bit more, it makes me wonder if there are any new functional changes related to configuration on the horizon. If so could those require new APIs? If that were the case it may be prudent to wait on this change and consider it w/ that new functionality.
--
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
[GitHub] [accumulo] ctubbsii merged pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
ctubbsii merged pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#discussion_r643285050
##########
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:
I think we'd have to add a lot more plumbing changes to make something like `isSet` or anything else in a dedicated type useful. Personally, I think Map is more than sufficient for now.
--
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
[GitHub] [accumulo] milleruntime commented on a change in pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#discussion_r646570300
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java
##########
@@ -609,16 +609,37 @@ void removeProperty(String tableName, String property)
* 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.
+ * can be used if consistency is required. Method calls getConfiguration and then calls
Review comment:
Could add a javadoc links to the `getConfiguration()` method here and in `NamespaceOperations`.
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#discussion_r643281439
##########
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:
To be consistent with those, getTableConfiguration and getNamespaceConfiguration might be most appropriate, but those are quite long, so I'm not 100% sold on those.
--
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
[GitHub] [accumulo] foster33 commented on pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
foster33 commented on pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#issuecomment-853094692
In my commit [f16dbf6](https://github.com/apache/accumulo/pull/2122/commits/f16dbf62c9791252254a4432b205fba991dbc34f), I have:
- Re-added the `@since` tag to the deprecated `@getProperties` method
- Changed the name of the new function to `@getConfiguration`
- Removed the duplicated logic in `@getProperties` and made it call `@getConfiguration` and return with .entrySet()
- Cleaned up 'clunky' code by implementing Map.forEach() or streams
- Made small changes to some variable types that were previously Iterables
In [ed2ce01](https://github.com/apache/accumulo/pull/2122/commits/ed2ce0156bf75a280d43f36592be82e1ca52ed6d) I added some parts to the deprecated tags that I had overlooked.
--
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
[GitHub] [accumulo] milleruntime commented on a change in pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#discussion_r644189636
##########
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:
I just noticed that the RFile API has two different builder methods that take properties as a parameter (one that takes a map and one that takes an iterator). https://github.com/apache/accumulo/blob/2ff2618b1c49d01a3ded44173b522c5670cfdeac/core/src/main/java/org/apache/accumulo/core/client/rfile/RFile.java#L185
--
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
[GitHub] [accumulo] foster33 commented on pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
foster33 commented on pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#issuecomment-854830155
In my commit [7d2583e](https://github.com/apache/accumulo/pull/2122/commits/7d2583ee4563f64b75dca7e61349f17187e0cc9f), I have:
- Changed some places to use Map.copyOf() instead of a Map.forEach()
- Changed lines of code to use the original `getProperties` method to shorten length of code
- Removed the deprecation tags for `getProperties()`
- Made `getProperties()` default in both `TableOperations` and `NamespaceOperations`
- Removed implementing methods of `getProperties()`
--
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
[GitHub] [accumulo] keith-turner commented on a change in pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#discussion_r643205304
##########
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:
When thinking about the name, may want to consider the methods in [the methods in InstanceOperations](https://github.com/apache/accumulo/blob/8a636a3ba91f5dae1d8b09b095178889a7d79c1d/core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java#L69-L77)
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#discussion_r644986541
##########
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:
I'm still in favor of deprecating. Some of the objections so far have been against removing, not against deprecating. The bar for deprecating should be much lower. I think API bloat is a harm that should be considered, and I think the SemVer process mitigates against the concerns over the worst effects of disruption. I would like to clean up API bloat over time, to streamline and simplify our API for ease of use in future, as well as accommodate existing users. I think the deprecation process with SemVer helps balance those competing goals, and think that some of the objections would have our API bloat over time. But, I think API evolution should also include the loss of vestigial APIs.
However, as it stands, I'm outvoted here. So, unless others agree with me, that we should slowly phase out the old vestigial API, using deprecation as a start, we should keep both and not deprecate at this time.
--
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
[GitHub] [accumulo] milleruntime commented on pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#issuecomment-856001314
@EdColeman I know you are working on a big change with the properties. Do you want me to wait and merge this change?
--
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
[GitHub] [accumulo] milleruntime commented on a change in pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#discussion_r641479658
##########
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:
I am wondering if we _need_ to mark this method deprecated. I can't think of a risky/harmful reason why a user shouldn't use this method. Yes, a map is more convenient, which makes the new method nice but I don't know if we need to mark this as deprecated. If a user is using it today there is no reason why they should change their code unless they want to.
--
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
[GitHub] [accumulo] keith-turner commented on a change in pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#discussion_r643320270
##########
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:
> getTableConfiguration and getNamespaceConfiguration
Yeah those are long. The suggestion of `getConfiguration` is more similar to those.
--
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
[GitHub] [accumulo] milleruntime commented on a change in pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#discussion_r643201084
##########
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:
I marked it on the PR since I think it warranted discussion. But after some more thought, I think it should be deprecated as well. It does feel like a mistake that the API didn't originally return the more flexible object.
--
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
[GitHub] [accumulo] Manno15 commented on a change in pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#discussion_r641535451
##########
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:
I think the initial reasoning that was detailed in the JIRA ticket was bloating the API. In this case, it isn't much bloat but that was the reasoning given in that discussion.
--
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
[GitHub] [accumulo] foster33 commented on a change in pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
foster33 commented on a change in pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#discussion_r644016098
##########
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:
> Yeah those are long. The suggestion of `getConfiguration` is more similar to the ones in InstanceOperations.
I have gone with `getConfiguration` . I think the consensus was this was the most appropriate in this situation.
--
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
[GitHub] [accumulo] milleruntime commented on a change in pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#discussion_r641479658
##########
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:
I am wondering if we _need_ to mark this method deprecated. I can't think of a risky/harmful reason why a user shouldn't use this method. Yes, a map is more convenient, which makes the new method nice but I don't know if we need to mark this as deprecated. If a user is using it today there is no reason why they should change there code unless they want to.
--
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
[GitHub] [accumulo] milleruntime commented on a change in pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#discussion_r644202126
##########
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:
Since there are two use cases for each type, it might be better to have both methods. Also, the method that returns the iterable is used quite a lot across the Accumulo code (a quick search showed me 51 places). I guess today I am back to not deprecating the method. If the implementation uses the same underlying method and without something better (like a new type that supports `isSet()`) it doesn't seem necessary to deprecate. Sorry, API design is difficult!
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#discussion_r645726127
##########
File path: core/src/test/java/org/apache/accumulo/core/clientImpl/TableOperationsHelperTest.java
##########
@@ -144,10 +144,15 @@ public void removeProperty(String tableName, String property) {
@Override
public Iterable<Entry<String,String>> getProperties(String tableName) {
+ return getConfiguration(tableName).entrySet();
+ }
Review comment:
This method can just be deleted now, since the default method exists in the interface.
--
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
[GitHub] [accumulo] foster33 edited a comment on pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
foster33 edited a comment on pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#issuecomment-853094692
In my commit [f16dbf6](https://github.com/apache/accumulo/pull/2122/commits/f16dbf62c9791252254a4432b205fba991dbc34f), I have:
- Re-added the `@since` tag to the deprecated `@getProperties` function
- Changed the name of the new function to `@getConfiguration`
- Removed the duplicated logic in `@getProperties` and made it call `@getConfiguration` and return with .entrySet()
- Cleaned up 'clunky' code by implementing Map.forEach() or streams
- Made small changes to some variable types that were previously Iterables
In [ed2ce01](https://github.com/apache/accumulo/pull/2122/commits/ed2ce0156bf75a280d43f36592be82e1ca52ed6d) I added some parts to the deprecated tags that I had overlooked.
--
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
[GitHub] [accumulo] milleruntime commented on pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#issuecomment-855998795
Thanks for the contribution @foster33 and welcome to the Apache Accumulo community! If you would like to be listed as a contributor on Accumulo's people page please make a website PR for people.md. https://github.com/apache/accumulo-website/edit/main/pages/people.md
If you intend to make more contributions, please consider subscribing to the dev list and introducing yourself. Also, you can use the dev list or slack to reach out if you have questions about anything. https://accumulo.apache.org/contact-us/
--
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
[GitHub] [accumulo] keith-turner commented on a change in pull request #2122: Change getProperties in TableOperations and NamespaceOperations to return Map instead of Iterable
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2122:
URL: https://github.com/apache/accumulo/pull/2122#discussion_r643320270
##########
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:
> getTableConfiguration and getNamespaceConfiguration
Yeah those are long. The suggestion of `getConfiguration` is more similar to the ones in InstanceOperations.
--
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