dlmarion commented on code in PR #5865:
URL: https://github.com/apache/accumulo/pull/5865#discussion_r2352426609
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -1684,7 +1684,8 @@ public static boolean
isValidResourceGroupPropertyKey(String property) {
return property.startsWith(Property.GENERAL_PREFIX.getKey())
|| property.startsWith(COMPACTION_PREFIX.getKey())
|| property.startsWith(COMPACTOR_PREFIX.getKey())
- || property.startsWith(SSERV_PREFIX.getKey()) ||
property.startsWith(TSERV_PREFIX.getKey());
+ || property.startsWith(SSERV_PREFIX.getKey()) ||
property.startsWith(TSERV_PREFIX.getKey())
+ || isFixedZooPropertyKey(getPropertyByKey(property));
Review Comment:
> It's not clear to me why the entire set of isFixedZooPropertyKey needs to
be added here.
I don't remember why I added `isFixedZooPropertyKey` here. I removed it in
b00d4d2.
> Right now, the set of valid Zoo properties seems to allow table
properties, which I thought was removed. Is that still pending in another PR?
#5781 changed the code to disallow table properties at the system level.
> I also think we need to refactor this a bit to have a set of valid
system-level properties (for resource groups and regular system-wide
properties), and keep that separate from what's valid for table/namespace
properties.
I agree with this, but I think it might be useful to do in another PR.
Personally, I think we should rename `isValidZooPropertyKey` to something like
`isMutableProperty`.
--
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]
