Adds property validation for instance.volumes property

[keith-turner]

No description provided.

[keith-turner]

This is an initial stab at making the property validation more strict for an important property that currently accepts any string. There are some other properties that could be made more strict, but would probably require introducing more single use property types. This change introduced PropertyType.VOLUMES that is only used by a single property.

[keith-turner]

The test are failing because the default value for instance.volumes is empty string which is not a valid value for this property. The property has no valid default but must be set to something that is valid at runtime.

[ctubbsii]

I think this code could be cleaned up by putting the validation in PropertyType.java (see the Predicate inner classes in there for reference).

By the time we get to the VolumeManager, the property should be guaranteed to be valid, so the code that redundantly checks the validation there could be simplified.

[ctubbsii]

I'm not sure how best to express the "empty is valid if only empty in the DefaultConfiguration, but not anywhere else" criteria that the tests are failing on. Typically, we have validators for the content of the property, based on the property type, and that's independent from the validation of the overall config, which might check several properties, or that some properties are set that are required, etc. So, for the PropertyType validator, it should probably just check that it is of the proper form or that it is empty (make empty valid) and then check that it's non-empty elsewhere.

[kevinrr888]

Related thread:
#5348 (comment)

[ctubbsii]

I think this code could be cleaned up by ...

Okay, after looking more closely, I can see now why it was being done this way... because it's relying on some of the private methods for normalization of URIs to do some of the validation. This is trickier than I first thought.

[keith-turner]

Okay, after looking more closely, I can see now why it was being done this way... because it's relying on some of the private methods for normalization of URIs to do some of the validation. This is trickier than I first thought.

Another reason I structured the code the way it is was to avoid making assumptions about exceptions thrown in another class. The code calls some private methods in the same class that throw a specific exception that is catches.

I have not figured out a good way to do it, but would like to somehow accomplish the following goals.

• Be able to validate volumes independently of creating a server process if possible. Like validate config in a stand alone utility.
• Support the concept that there is no default value but to be valid config for a system it must be set to a list of one ore more URIs.
• If possible have the validation code reside in Property and PropertyType instead of off at some random place in the code.

[ctubbsii]

I'm about to add a commit to this PR which I think helps some of these things.

• Be able to validate volumes independently of creating a server process if possible. Like validate config in a stand alone utility.

I put the parsing code for volumes into ConfigurationTypeHelper and added a unit test to help support anything that needs to parse it, either for use or for validation.

• Support the concept that there is no default value but to be valid config for a system it must be set to a list of one ore more URIs.

This one is tricky, because the PropertyType validation is tied to the Property enum which is so tightly coupled to our AccumuloConfiguration (including DefaultConfiguration). We have a separate ConfigCheckUtil, and maybe other places in the code that maybe could do validation of the configuration as a whole, like ensuring a property is set because it's a required property. But the PropertyType validation is really supposed to validate the format, if set. Making it required to be set is going to need to be done elsewhere, if it's not already done.

• If possible have the validation code reside in Property and PropertyType instead of off at some random place in the code.

I moved the validation to PropertyType

[keith-turner]

@ctubbsii these changes look good. I added one more test because the code in ConfigurationTypeHelper and VolumeConfigugration work together to handle some of the empty cases. The added test just covers this case.