ctubbsii commented on issue #1665: URL: https://github.com/apache/accumulo/issues/1665#issuecomment-672396552
@keith-turner Right, I got that far already. My concern wasn't exactly a question of where to implement it, because I did already track down that code after a significant amount of time. Rather, my concern is in the difficulty in tracking it down itself, as a result of the current design, as well as potential problems with the overall design of this new configuration parsing stuff for compactions. The core of my concern is this: While other uses of configuration properties are fairly easy to track down, because either the enum or their configuration key is directly referenced in other code, that is not the case for these new compaction properties. These configuration properties are utilized in the code only after being carefully parsed by code that is limited in comments and has no direct references to any of the configuration enums or keys themselves (which would make it much easier to search for references and to ensure changes are correctly made in all places the property is used, if a change is needed; aka maintainability). Instead, these enums seem to exist solely for the documentation generator, and are largely disconnected from the implementation. This somewhat undermines the point of the enum container for configuration properties, as the enums exist to help avoid bugs that can be introduced due to implementation code that diverges from the documentation. I also thi nk that the complexity, while I admire the JSON-based solution you designed, is partially to blame for the unexpected behavior at the core of this ticket. And, I suspect this ticket won't be the last of its kind pertaining to these properties if we do not adequately address it sooner. So, I realize that's a fairly critical view of your code... probably coming across more critical than I intend (sorry, if that's the case). But that's why I was hoping to have a live chat... to see if we can come up with something that alleviates my concerns without undermining the intent of your design. I can, of course, bake in a fix for the narrow issue identified in this ticket, but I was hoping to discuss the overall approach. With a better overall approach, I'd have more confidence in a fix for this issue. As it stands, without being certain of everywhere in the code where these properties are used (and it being hard to find them), I'd have a low degree of confidence in the overall correctness of any fix (for this issue, and any others with regard to these configuration properties). Testing can help, but being able to follow the code with a better overall more readable/maintainable design, would also help. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected]
