ctubbsii commented on code in PR #5908:
URL: https://github.com/apache/accumulo/pull/5908#discussion_r2370315607
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader11to12.java:
##########
@@ -936,6 +936,19 @@ void moveTableProperties(ServerContext context) {
final Map<String,String> nsPropAdditions = new HashMap<>();
for (Entry<String,String> e : sysTableProps.entrySet()) {
+
+ // Don't move iterators or constraints from the system configuration
+ // to the system namespace. This will affect the root and metadata
+ // tables.
+ if (ns.equals(Namespace.ACCUMULO.name())
+ &&
(e.getKey().startsWith(Property.TABLE_ITERATOR_PREFIX.getKey())
+ ||
e.getKey().startsWith(Property.TABLE_CONSTRAINT_PREFIX.getKey()))) {
+ LOG.debug(
+ "Not moving property {} to 'accumulo' namespace, iterator and
constraint properties are ignored on purpose.",
Review Comment:
Could probably say why they are ignored: they can break the functionality of
the built-in tables, and should be added by users very carefully if they need
them.
I would say that warning would be better... but this preserves existing
behavior, so there's nothing to really notify the user about because there's no
action they need to take.
##########
server/base/src/main/java/org/apache/accumulo/server/conf/NamespaceConfiguration.java:
##########
@@ -40,47 +35,6 @@ public NamespaceConfiguration(ServerContext context,
NamespaceId namespaceId,
super(log, context, NamespacePropKey.of(namespaceId), parent);
}
- @Override
- public String get(Property property) {
-
- String key = property.getKey();
-
- var namespaceId = ((NamespacePropKey) getPropStoreKey()).getId();
- if (namespaceId != null && namespaceId.equals(Namespace.ACCUMULO.id())
- && isIteratorOrConstraint(key)) {
- // ignore iterators from parent if system namespace
- return null;
- }
Review Comment:
It looks like this would have skipped retrieving iterators or constraints
for metadata tables if they were set at the namespace level. This might have
been an existing bug that got inadvertently fixed by the removal of this method.
That was probably okay to have in place, since setting those on the accumulo
namespace would have been unusual.
It's also probably okay to remove it, since it's not necessarily a use case
we need to restrict.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
