ctubbsii commented on issue #3981: URL: https://github.com/apache/accumulo/issues/3981#issuecomment-1862545734
The way this is written is very confusing to me. I read it as moving properties from Property.java to accumulo.properties. However, Property.java is just an enum that helps us store property documentation, default values, and more that help us read the properties from an accumulo.properties file (or from properties stored elsewhere on the system). So, they aren't separate things... they go together. The accumulo.properties file you linked to is merely a reference for a relatively minimal implementation. I would not want the minimal implementation bloated with extraneous configs. But, I'm not sure if that's what you're suggesting, or if you're suggesting something else. There are some elements of this that I'd be in favor of: * Changing the default values to an empty string * Making sure the properties are referenced by the compaction code instead of only placed for documentation and not actually retrieved via the enum references There are also some related things that could help here: * Use "general.custom." or "table.custom." prefixes for pluggable implementations that provide complex, non-default features (see VolumeChooser subclass configs, or other SPIs that behave similarly) * Relax our constraint that non-core properties be prefixed with "general.custom." prefix, or "table.custom." prefix to be stored in our config files (but this means we need to be more rigid about the property namespace we do expect to be reserved for built-in properties) * Combine complex config into a single (possibly optional) property for customization (AWS uses this kind of pattern [extensively in its APIs](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ec2-volume.html)) * Wrap complex features into a pluggable component; users can choose to hard-code in the complexity, or make their implementation of that component customizable through config; we choose a simple implementation that requires no config (see also VolumeChooser implementations for a similar pattern, or our general context class loader factory, for an even simpler implementation of this). -- 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]
