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