apache
Adding support for setting multiple properties at once atomically
Here is a draft PR with my in progress changes for #2692 so it can start being reviewed.
I still need to do some more clean up (documentation and things), maybe some more tests and also the last piece of updating the shell commands to use the new features.
I plan to continue working on it later this week (probably this weekend).
I squashed the commits from #2717 and based on my PR commit on that PR.
@ctubbsii and @dlmarion - here is my draft PR, it's still a work in progress as I state in the description but there's quite a bit here so feel free to give feedback.
Looks like #2796 was merged so I will rebase and refactor this PR based on that when I get back to this. I'm planning to continue working on this PR again this weekend. I'll continue on with updating the tests, docs, and shell code. If we determine to change how the getConfiguration() stuff works I can fix that too.
@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.
@EdColeman - I just rebased off main since I saw you merged in your PropUtil changes and pushed up the latest
@ctubbsii, @EdColeman, @dlmarion - I just pushed an updated draft PR that has changes to handle detect and handle version changes when modifying properties and is ready for review again. I added a new commit for now just to make it easier to see what my latest changes were. I will rebase/squash the commits and update the commit message when it's ready for merging.
I took @keith-turner's suggestion and went ahead and updated the modifyProperties() methods to retry on concurrent modification exceptions up to 5 times before giving up and propagating the exception back to the caller of the method. We could also make this fail immediately and not retry if desired.
Let me know what you think of the latest updates.
@ctubbsii - This PR has been updated to remove the retry on ConcurrentModificationException
I went ahead and squashed my commits into 1 (leaving Emily's) and I also rebased and removed the draft status as I think this PR is about ready to merge after a final review.
I went ahead and squashed my commits into 1 (leaving Emily's) and I also rebased and removed the draft status as I think this PR is about ready to merge after a final review.
Squashing isn't necessary usually. We squash when we merge the final PR. Squashing and force pushing can make it harder to follow the history of the ticket while reviewing.
I went ahead and squashed my commits into 1 (leaving Emily's) and I also rebased and removed the draft status as I think this PR is about ready to merge after a final review.
Squashing isn't necessary usually. We squash when we merge the final PR. Squashing and force pushing can make it harder to follow the history of the ticket while reviewing.
Ok sounds good in the future I can leave the commits and squash later on merge. I still like force pushing when just rebasing though as nothing is changing with the commit itself (usually)
@Manno15 - I went ahead and restored my previous commit to make it easier to review the concurrent modification changes so the PR is back to 3 commits again.
I took @keith-turner's suggestion and went ahead and updated the modifyProperties() methods to retry on concurrent modification exceptions up to 5 times before giving up and propagating the exception back to the caller of the method.
@cshannon where in the code does it retry 5 times? I looked around for this and did not see it. A lot of things in Accumulo retry forever unless explicitly given a timeout. I think retrying 5 times is hard to reason about for a user, especially someone who just wants to props to be set and are not too concerned w/ how long it takes. They would have to write the code to retry and it would probably not be as optimal as if Accumulo was retrying (like a user could not optimally implement exponential back-off on top of code that does some sort of retrying on its own).
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.
I think it does make sense to update the shell command in a separate PR. It would be good to attempt that before any release that has this new API as a it will help validate the new API. We could possibly update the shell command to support a conditional update with this new API. Something like update props p1=v11,p2=v21, etc only if the current props are p1=v1 and p2=v2. This would allow a user to do the following.
- print the current props
- create a command that updates props only if those props have the values they saw when they printed.
Regarding @keith-turner suggestion - depending on where / how that is implemented the data version of the properties can be used to tell if the property node has changed. It may not be necessary to actually compare values.
@keith-turner - The retry was removed as it was discussed and determined to not be necessary or wanted and should just fail instead. I forget when but I believe it was an offline conversation with @ctubbsii and others.
A separate PR to update the shell would be a good idea if we want to do that to keep the scope of this PR smaller as it's already a decent sized change. I could add that as a follow on once his is merged.
@keith-turner - I think I misread your comment initially as I was thinking you meant to do the PR after this PR is merged (but before releasing 2.1) but I think you meant to go ahead and do the shell changes first. I can take a look at updating the command later this week or next week to enhance it to help validate this PR. I would need to do some experimenting with it and see how it looks from a user feel perspective.
Regarding @keith-turner suggestion - depending on where / how that is implemented the data version of the properties can be used to tell if the property node has changed. It may not be necessary to actually compare values.
I suggested comparing values thinking the data version was not available through the public API. But could still compare values through the public api. Also the suggestion for the shell was just a WAG about how it should be done. Not exactly sure how the shell command should look. My main point was that its really good to use a new API before releasing it.
I think I misread your comment initially as I was thinking you meant to do the PR after this PR is merged (but before releasing 2.1) but I think you meant to go ahead and do the shell changes first.
I wasn't thinking it needed to be done before merging this PR, just before release like you were initially thinking. One way we have handled a situation like this in the past is to create a new issue that is a blocker before merging this.
@keith-turner - The retry was removed as it was discussed and determined to not be necessary or wanted and should just fail instead. I forget when but I believe it was an offline conversation with @ctubbsii and others.
I think building in retry behavior is nice for the following reasons.
- The concurrent modification exception (CME) is spurious and therefore hard for a user to test. User would most likely only see it in a production system, not a test system.
- I suspect every user who uses the API would just want it to set the props even if there was CME (its a spurious condition that has nothing to do with the data). So that means each user who uses the API will have to rewrite some sort of retry code (retry code that is hard for them to test).
- If we build in the functionality then we can test it and write a more complex retry strategy like exponential back off that will not retry forever even when there is lot of concurrency. We can do the hard part instead of pushing that off on every user of the API.
However I don't think implementing retry should hold up this PR. I think it would be fine to do that as a follow on if there is no opposition to it.
We should keep the clause about eventually consistent until we can strengthen the use of versioning information so that there is a stronger guarantee. Its in the nature of a distributed system - but it still can be supersizing, and even causes problems in our tests if we set and then immediately expect that to be reflected across the running processes, treating it as some sort of atomic operation.
We should keep the clause about eventually consistent until we can strengthen the use of versioning information so that there is a stronger guarantee. Its in the nature of a distributed system - but it still can be supersizing, and even causes problems in our tests if we set and then immediately expect that to be reflected across the running processes, treating it as some sort of atomic operation.
That part is still there, I just removed "This new method returns a Map instead of an Iterable."
https://github.com/apache/accumulo/pull/2799/files#diff-55271a1466a3153c9fd3ed242bd3c37f38fd98c46a87ce1d2993f65d5606ca52R683-R687