Make several TableOpsImpl methods atomic

[DomGarguilo]

Fixes #5494
This PR replaces places were multiple calls to set or remove properties were being done with a single modify properties block to make things atomic. The affected methods are:

• setSamplerConfiguration
• clearSamplerConfiguration
• setLocalityGroups

[kevinrr888]

LGTM, code is also quite a bit more succinct now

[ctubbsii]

Why would this no longer throw TableNotFoundException? That seems odd.

[DomGarguilo]

Nothing in this method throws TableNotFoundException anymore I guess. In the old code, clearSamplerOptions() threw it when it called getProperties() but all of that has been replaced with the modifyProperties() block which does not throw TableNotFoundException.

[ctubbsii]

I would expect modifyProperties to throw TableNotFoundException. At least, some error should happen if the table doesn't exist... I'm not sure what error that is. But, I think this PR should wait until we figure that out, because this changes the behavior, and we shouldn't change the API behavior for the case when the table doesn't exist.

[DomGarguilo]

So it turns out some of the TableOps methods wrap TableNotFoundException in AccumuloException. Created #5595 to add a bit of clarity for those methods. I am not sure why some methods do this. It seems like it would be better to just throw it directly (might avoid confusion like this scenario).

Anyways, I dont think this is a blocker for this PR.

[DomGarguilo]

Why would this no longer throw TableNotFoundException? That seems odd.

@ctubbsii I added a new helper method, modifyPropertiesUnwrapped() which wraps modifyProperties() catches AccumuloException, and throws TableNotFoundException if thats its cause. I used this new helper method in only the methods that I changed here (setSamplerConfiguration, clearSamplerConfiguration, setLocalityGroups). This adds back the TableNotFoundException that had been removed by a previous change in this PR. It should also restore the exception handling functionality that was in place before converting these methods to use modifyProperties.

[ctubbsii]

The strategy to re-throw a TNFE by unwrapping the exception wrapped in modifyProperties is okay, but the implementation is not working as intended, since you're looking up the table ID. After that is fixed, this could probably be merged.

However, this is still just a hack, because modifyProperties doesn't throw TNFE when it should. A possible better solution is to deprecate it, and create a replacement method, mutateProperties that has the correct exception throwing behavior. So, instead of adding more unwrapping... you remove some of the existing wrapping that is happening in the first place. This would actually go well with the parameter name, which is "mapMutator", which works better with a method named "mutate" rather than "modify".

The problem with creating a new method is that we tend to avoid doing that in bugfix releases, because it violates semver. However, the spirit of semver is to avoid breaking user code. Although this would add a new method, I think that the only code that would break is subclasses, but nobody else is likely to be subclassing the TableOperations interface. So, maybe it is tolerable to add this in a bugfix? If not, your strategy is fine for 2.1 We can fix it the way I'm suggesting in 4.0, and add a deprecation to the old method in a 3.1.

[DomGarguilo]

However, this is still just a hack

Yea, definitely a hack and should just exist temporarily until we properly fix things.

So, maybe it is tolerable to add this in a bugfix? If not, your strategy is fine for 2.1 We can fix it the way I'm suggesting in 4.0, and add a deprecation to the old method in a 3.1.

Right now the changes in this PR are made against 4.0. Are you suggesting that we make these changes against 2.1 too?