WIP: Property Validation handling on backend #3173
Conversation
Refactored the isTablePropertyValid method to be mroe generic for other usecases. Added 'throws' back to method signatures in partial attempt to improve error visibilites.
Attempt at raising exceptions via thrift for invalid properties.
| 2:SecurityErrorCode code | ||
| } | ||
|
|
||
| exception ThriftPropertyException { |
There was a problem hiding this comment.
No need to put this exception in the client thrift IDL file. You can just put it in the manager.thrift file unless you need to import it into other IDL files.
| } catch (Exception e) { | ||
| Manager.log.error("Problem setting config property in zookeeper", e); | ||
| throw new TException(e.getMessage()); | ||
| throw new ThriftPropertyException(property, value, e.getMessage()); |
There was a problem hiding this comment.
Do you want this above where IllegalArgumentException is caught?
There was a problem hiding this comment.
Yes, I moved up my Thrift property Exception to handle the caught iae
| * valid see {@link #isValidPropertyKey} and that the value is a valid format for the type see | ||
| * {@link PropertyType#isValidFormat}. | ||
| * | ||
| * @param key property key | ||
| * @param value property value | ||
| * @return true if key is valid (recognized, or has a recognized prefix) | ||
| */ | ||
| public static boolean isTablePropertyValid(final String key, final String value) { | ||
| public static boolean isValidProperty(final String key, final String value) { |
There was a problem hiding this comment.
I haven't done a full review of this PR, but it seems weird that this method would have had the name "Table" in it if it weren't specifically constraining the validity check to properties that can be set scoped to a table (or namespace). We should be careful about changing this name, and its current behavior that constrains validity to properties whose keys are appropriate for a table (or namespace). There's a good chance that this method is expected to enforce that constraint, and now it no longer does.
There was a problem hiding this comment.
I agree that the previous method was constrained to only checking table validity.
It was accomplishing this by checking if the property was valid and then also doing a call to isValidTablePropertyKey to determine if the property key itself was valid for tables.
However, since isValidTablePropertyKey is public, I refactored all usages of isTablePropertyValid with two separate method calls to isValidProperty and isValidTablePropertyKey.
I did this because I wanted to try and streamline the Property.java class.
This class had this method (isTablePropertyValid) along with isValidTablePropertyKey and isValidBooleanPropertyKey.
If we followed this pattern, we would end up with a validity check for each property type [PropertyType.java
as well as validity checks for table, namespace, and system key types.
The end state would be a cluttered class with a lot of different isValid<X> methods that has the potential to be confusing.
I chose to streamline down to a single isValid method and enforce the context check of "table" in the usage location.
However, if those extra methods are the desired goal then I can pivot and create a new generic isValidProperty method while leaving isTablePropertyValid intact?
Validate System properties as well as checking if property is mutable in zookeeper.
Added formatting change
Adds tests for setting invalid namespace and table properties.
3 participants
I'm looking for feedback on direction of thrift error handling.
I'm not seeing the errors I'm expecting, so I'm attempting to figure out what disconnect I still have
My expectation is that I would have ThriftPropertyExceptions returned for any
accumulo shell config commandthat sets an invalid property.However, I'm only getting exceptions back for setting properties on tables and namespaces.
If I set a direct property without specifying a table or namespace, no exception is raised
Invalid Table Property Example:
Invalid Namespace Property Example: