remove extra node in prop path

[EdColeman]

The original rework of the prop store was using a separate ZooKeeper node under the "original" configuration node. In ZooKeeper the structure looked like:

/accumulo/[instance id]/config/encoded_props  <-- system props
/accumulo/[instance id]/namespaces/[nid]/conf/encoded_props  <-- namespace props
/accumulo/[instance id]/tables/[tid]/conf/encoded_props  <-- table props

This removed that extra node an stores the configuration directly in the configuration node. The structure now looks like:

/accumulo/[instance id]/config  <-- system props
/accumulo/[instance id]/namespaces/[nid]/config  <-- namespace props
/accumulo/[instance id]/tables/[tid]/config  <-- table props

[ctubbsii]

I do think it would be nice if system scope used the same node name (config or conf, consistently). We could also fix that here before the release.

[ctubbsii]

This would fix #2690

[EdColeman]

Changing the node name so that they match would require additional changes that would include removing the legacy node. It was easier now to keep the existing convention. I would suggest a follow-on PR. It is mainly going to be changes in the ConfigurationTransform code.

A higher priority right now should be working to trace down the concurrency issues with multiple property updates - once those are resolved, this could be revisited.

[cshannon]

Changing the node name so that they match would require additional changes that would include removing the legacy node. It was easier now to keep the existing convention. I would suggest a follow-on PR. It is mainly going to be changes in the ConfigurationTransform code.

A higher priority right now should be working to trace down the concurrency issues with multiple property updates - once those are resolved, this could be revisited.

Tests pass with the latest in PR #2967, See my comment here: #2967 (comment)

[cshannon]

This change doesn't make sense to me to merge in now as it seems too late to be making a change like this with the 2.1.0 release being imminent. I would think it would be better to wait until the next version for this change and at the same time we can also rename system scope to be consistent with the others.

[cshannon]

Also if the reason to do it now would be that changing it later requires doing some upgrading/migration to the property store then that could just be part of the migration that needs to be done anyways if we rename the system level node from config to conf.

[cshannon]

What's the status of this PR now? Is there anything else to do here or is it ready?

[EdColeman]

I have an update forthcoming that uses the same node name (config) instead of config for system and conf for namespace and tables - going through the final clean-up now. Because of merges, the push will likely be a force push.

[EdColeman]

This now contains the changes needed to use config instead of conf as the configuration node name for tables and namespaces (system already used config)

[cshannon]

I think this mostly looks good but I think a couple things could be changed which I left comments for.

I also think some of the testing should be improved a bit specifically for ConfigTransformer and ConfigProperyUpgrader. Since this is upgrading properties I think it should be well tested as it wouldn't be great if the upgrade logic failed and broke an existing install when a user upgraded.

1. ConfigProperyUpgraderTest was created but just uses a mock ConfigTransformer. It also doesn't test much other than the normal path. There's some edge cases that could be tested I think to make sure it handles errors. For example test what happens if the legacy path is bad, etc.
   2.For ConfigTransformer, there are a couple integration tests (only ConfigTransformerIT) but not many. I think it might be useful to create a test specifically for ConfigTransformer (just like you did for ConfigPropertyUpgrader) to test the new behavior (using mocks, etc) and run through the different edge cases and scenarios to verify it works.
   3.Lastly, I think both ConfigPropertyUpgraderIT and ConfigTransformerIT could use some extra tests to exercise the new behavior in more scenarios.

[cshannon]

LGTM now

[dlmarion]

LGTM