[#6750] improvement(core): property framework supports prefix #6751
Conversation
mchades
force-pushed
the
oss-prop-prefix
branch
2 times, most recently
from
44b4462 to
9d8cec6
Compare
mchades
force-pushed
the
oss-prop-prefix
branch
2 times, most recently
from
5244adf to
597e34e
Compare
| .collect(Collectors.toList()); | ||
| Preconditions.checkArgument( | ||
| absentProperties.isEmpty(), | ||
| "Properties are required and must be set: %s", | ||
| "Properties or properties with a fixed prefix are required and must be set: %s", |
There was a problem hiding this comment.
I can't get Properties or properties with a fixed prefix are required. According to your code, it should be Required properties should be set or required prefix properties should be set with prefixes in the key.
There was a problem hiding this comment.
To be more accurate, prefix properties should be a property with a fixed prefix, then you can understand the above description well
| * @return the property entry object of the property. | ||
| * @throws IllegalArgumentException if the property is not defined. | ||
| */ | ||
| default PropertyEntry<?> getPropertyEntry(String propertyName) throws IllegalArgumentException { |
There was a problem hiding this comment.
Can you provide more comments on this method? will we get the value of the longest key for prefix property and why?
| @@ -125,6 +125,8 @@ void testCatalogProperty() { | |||
| throwable | |||
| .getMessage() | |||
| .contains( | |||
| String.format("Properties are required and must be set: [%s]", METASTORE_URIS))); | |||
| String.format( | |||
| "Properties or properties with a fixed prefix are required and must be set: [%s]", | |||
There was a problem hiding this comment.
"Properties or property prefixes..."
| : propertyEntries().entrySet().stream() | ||
| .filter(e -> e.getValue().isPrefix() && propertyName.startsWith(e.getKey())) | ||
| // get the longest prefix property | ||
| .max(Map.Entry.comparingByKey(Comparator.comparingInt(String::length))) |
There was a problem hiding this comment.
What's the meaning here, can you explain more?
| return propertyEntries().containsKey(propertyName) | ||
| && propertyEntries().get(propertyName).isReserved(); | ||
| // First check non-prefix exact match | ||
| PropertyEntry<?> directEntry = propertyEntries().get(propertyName); |
| * <p>For example, if there are two property prefix entries "foo." and "foo.bar", and the property | ||
| * name is "foo.bar.baz", the entry for "foo.bar" will be returned. If the property entry | ||
| * "foo.bar.baz" is defined, it will be returned instead. |
There was a problem hiding this comment.
Is it a valid scenario? Do we need to allow such scenario or ban this?
There was a problem hiding this comment.
we already have the scenario, see the chained authorization plugin properties:
property authorization.chain.plugins and property prefix authorization.chain. have the same prefix
| * @return the non-prefix property entry object of the property. | ||
| * @throws IllegalArgumentException if the property is not defined or is a prefix. | ||
| */ | ||
| default PropertyEntry<?> getNonPrefixEntry(String propertyName) throws IllegalArgumentException { |
There was a problem hiding this comment.
Better to use Optional instead of throw and catch an exception.
What changes were proposed in this pull request?
property framework supports prefix
Why are the changes needed?
some properties with fixed prefixes maybe immutable or required and need to be verified.
Fix: #6750
Does this PR introduce any user-facing change?
no
How was this patch tested?
tests added