cshannon commented on PR #2799: URL: https://github.com/apache/accumulo/pull/2799#issuecomment-1179584479
@EdColeman and @dlmarion - Ok I have updated my PR with a few changes to address the comments/discussion made as well as added/changed some tests. I rebased my PR off main and cherry-picked the commit from #2803 so that it will be easy to rebase when #2803 is merged. I then fixed up my PR to incorporate the changes made in https://github.com/EdColeman/accumulo/tree/prop_atomic_update . I also ran through the Sunny day test profile and everything passed. **To address Ed's comments:** - I renamed everything to be consistent so getSystemProperties(), getTableProperties(), getNamespaceProperties(), etc as we are only returning properties and not the full config. - For system properties I updated the PR to be handled the same was as table properties and namespace properties. Same naming convention and new method on instanceOperations() etc. There may be work to do for here in another PR as stated if we want to deduplicate things - For the exception handling I think it makes sense to fail fast on the first invalid property and throw an exception. It looks like the same IllegalStateException is thrown already on setting an invalid property as things get validated so the behavior is the same. - For the tests I went and updated/added some more. I tried to do a search and add some tests in various areas to cover functionality (adding, removing, etc) and I also tried to add some tests for permissions. My guess is the tests may not be complete and I may need to review it and write some more edge cases tests but at least what is there should demonstrate the basic functionality. **Other notes:** - I reverted the changes Emily did to the Config command. It doesn't really make sense to try and mutate multiple properties at once with a command since it would blow stuff away. If we do want to have a command I would think we would need an entirely new command (not mess with the current one) and that is probably best with a follow on PR. Maybe we just have a way to script or upload a set of properties. - The javadocs probably need some work and review for accuracy. I figured I would go back and fix up the Javadocs after the next review and when things look good before the final merge to make sure I'm not spending a lot of time on Javadocs for things that will be removed/deleted anyways. -- 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]
