Property.isValidZooPropertyKey seems incorrect/outdated

[kevinrr888]

There are several prefixes not defined in isValidZooPropertyKey, which I'm not sure is by design. For example, should isValidZooPropertyKey include any of RPC_PREFIX, INSTANCE_PREFIX, MONITOR_PREFIX, COMPACTOR_PREFIX, etc? It seems like FIXED_PROPERTIES is meant to be a subset of those that are isValidZooPropertyKey (see descriptions for isFixedZooPropertyKey and isValidZooPropertyKey). However, there are several cases of a fixed property which would not be isValidZooPropertyKey (e.g., COMPACTOR_PORTSEARCH is in FIXED_PROPERTIES but would return false for isValidZooPropertyKey). Either

1. At least one description is incorrect and should be updated
2. isValidZooPropertyKey is missing some prefixes and should be updated

2 seems most likely

Originally posted by @kevinrr888 in #5632 (review)

[dlmarion]

I think the use for isValidZooProperty is to deny setting any property in ZooKeeper that is process specific. This would include instance properties and properties that the user might set in their site file for a specific instance of a process. For example, a user may want to use different block cache sizes on different servers due to a heterogeneous environment. Setting the block cache size in ZK, I think, would override these settings.

Based on the current contents of the isValidZooProperty method, it looks like the intent is to allow everything but the instance properties. It looks like we allow setting other process properties (like block cache) in ZooKeeper, and leave it up to the user to make the correct decision for their deployment.

[ctubbsii]

I'm not sure about RPC_PREFIX or MONITOR_PREFIX, but those are probably okay to exclude. I haven't fully thought through the risks of allowing them there, but don't think anybody is asking for them to be configurable in ZK, so it's fine that they aren't. If there's a feature request, we can consider adding them.

INSTANCE_PREFIX is required to be excluded. It contains bootstrapping properties, and it makes no sense to store them in ZK... they wouldn't be usable there.

COMPACTOR_PREFIX must be excluded for now. These would override per-resource group configuration files, and we don't have the ability to store resource group-specific configs in ZK yet. If we added that, then these could be allowed. This also applies to other server types that can be placed in resource groups, but since those predate resource groups, we're stuck with the historical behavior.

"Fixed" properties are, by definition, those ZK-configurable properties that are affixed to a specific value on first read, and require a restart for changes to take effect. So, by definition, they are a subset of the ZK-configurable properties.

COMPACTOR_PORTSEARCH probably got caught up in ensuring all *_PORTSEARCH properties got added to the fixed properties, without consideration of the fact that COMPACTOR_PREFIX isn't allowed (for the reasons already stated). Since the fixed properties only matter if it's a zoo-configurable property, then it's probably not causing any harm, but would probably be good to make this consistent and remove it from the list for consistency.

I don't think there's anything to do for this one, unless there's demand for a feature request or to trivially fix the COMPACTOR_PORTSEARCH issue. I think it is not outdated or incorrect, in general. But, it would be easier to maintain them if the ideas on #5693 were complete.