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