ctubbsii commented on issue #5693: URL: https://github.com/apache/accumulo/issues/5693#issuecomment-3050321810
> My understanding of the situation is: > > 1. Most properties can be modified at runtime > 2. Whether or not the code responds to the update actually determines whether it's "fixed" or not All fixed properties are prevented from responding to the update, based on either pre-existing implementation (such as them only having been read once at process start) or because of the risks associated with inconsistency. The current implementation mitigates those risks of inconsistency, but it's possible there are better ways to mitigate those risks (for example, by establishing more strict lifecycles for each property usage and ensuring they are only used in accordance with that lifecycle) but the current implementation to cache the first value seen for the lifetime of the process seems the simplest. > 3. The fixed set in Property.java is the current understanding of which properties respond to change > 4. The fixed set, which could be wrong, is used to log a warning when the property is changed > > I'm not sure that adding a field to the constructor is going to fully fix the issue. It alleviates us from having to maintain a separate data structure, but it could still be out of date if the code that uses the property changes at a later point. Adding an item to the fixed properties set forces the behavior due to the current caching. Adding it to the constructor would force us to think about whether it should or should not be in the set. This would force the list to be up-to-date, and our caching forces consistency between behavior and documentation. If we want to change our mind, we can reevaluate its inclusion in the list easily by changing the parameter constructor and any related code, and the enforcement of the behavior would be automatically handled. > > A different approach to this could be: > > 1. Disallow changes to truly fixed properties (instance, rpc, etc.). This set may be the same set of properties that should not be set in ZooKeeper. That would be really limiting > 2. Allow any other property to be modified That could cause a bunch of problems. One example: we have no way to specify properties by resource group in ZK, so any ZK property would override any per-resource group config file. > 3. Log warnings when a property has been changed, but not re-read after some period of time. That's the current behavior (but "some period of time" is the lifetime of the service). > However, I'm not sure if that's entirely possible given how the properties are stored in ZooKeeper. Would require some investigation. ***** The "fixed" properties are a strict subset of the zoo-configurable properties that are "affixed" to the first value read from the config. This only makes sense for zoo-configurable properties, because other configurations are immutable. Since the three options are: 1. Zoo-Configurable 2. Zoo-Configurable, but requires restart (aka "fixed") 3. Not Zoo-Configurable We could probably simplify the three cases by adding an enum to the Property constructor, but it should probably express all 3 possibilities, instead of just "fixed" or "not fixed". -- 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]
