Check Prop Value Format and Key types.

[ddanielr]

relates to #3145

The shell config command was only checking Property Key Types and not Value Formats on Set operations. Changed Set operation to also validate value types to ensure proper format before setting properties.

This changes the shell client behavior to now throw an error message to the user on an invalid property setting.

Boolean Example:

root@uno> config -s tserver.port.search=123
2022-12-30T15:14:21,363 [shell.Shell] ERROR: org.apache.accumulo.core.util.BadArgumentException: Invalid Property value (requires type: boolean) near index 30
config -s tserver.port.search=123

Port Example:

root@uno> config -s tserver.port.client=65539
2022-12-30T15:15:29,328 [shell.Shell] ERROR: org.apache.accumulo.core.util.BadArgumentException: Invalid Property value (requires type: port) near index 30
config -s tserver.port.client=65539

This PR is staged against main, but we may want to backport this change.

[ctubbsii]

Ideally we'd validate property values on the server side, and reject the RPC request (and propagate the error through the public API call), when validation fails.

[ctubbsii]

I think the fix, when ready, can be applied to the 2.1 branch. This PR is currently targeting the main branch. If you cherry-pick the changes to a 2.1 bugfix branch, and force push to the branch that is backing this PR, we can edit the PR's base branch to make this a PR against 2.1.

[keith-turner]

If possible, it would be nice to add an IT to the ShellIT that exercises setting a prop w/ a bad value.

[keith-turner]

Ideally we'd validate property values on the server side, and reject the RPC request (and propagate the error through the public API call), when validation fails.

Is there a bigger problem here? Re my comment earlier about testing the shell, it might be nice to have an IT for the public API also that tries to set a bad value if we do not have one.

[ddanielr]

Ideally we'd validate property values on the server side, and reject the RPC request (and propagate the error through the public API call), when validation fails.

Yep, I'm working on that now. I think it's also valuable to have the shell perform validation and not issue the RPC request in the first place.

[EdColeman]

It looks like instead of tableName the parameter is more generic name (either table name of namespace name)

[ddanielr]

I'm not sure how to determine the best fix for this since namespace and table don't have good property separation.

Only ValidTableProperties are defined in Property.java.
There is not a separate ValidNamespaceProperties hashset defined.

accumulo/core/src/main/java/org/apache/accumulo/core/conf/Property.java

Lines 1378 to 1381 in 339672a

	private static final HashSet<String> validTableProperties = new HashSet<>();
	private static final HashSet<String> validProperties = new HashSet<>();
	private static final HashSet<String> validPrefixes = new HashSet<>();
	private static final HashMap<String,Property> propertiesByKey = new HashMap<>();

You can also see the interchangeability in the property set code for namespace. It's just checking if it's a valid table property vs anything namespace specific.

accumulo/shell/src/main/java/org/apache/accumulo/shell/commands/ConfigCommand.java

Lines 105 to 106 in 339672a

	} else if (namespace != null) {
	if (!Property.isValidTablePropertyKey(property)) {

So, should namespaces have separate properties that aren't table properties?
if so, I can rework this and also add a isValidNamespacePropertyKey method to Property.java

[EdColeman]

Properties are a hierarchy default <- system <- namespace <- table with the more specific overriding the more generic. I'm not sure of the top of my head, but I expect that all table properties are valid namespace properties. There may be a few specific system only props, but in general they should also be override-able in a namespace or table.

[ddanielr]

I've thought about these changes and I'm closing this PR in favor of the fix in #3177