[#1221] improvement(common): Add the method checkValue for the ConfigEntry
#1233
Conversation
|
Probably be better to add the |
OK, I will try to add more checks. |
4d3182c to
11ea369
Compare
Added. |
| StringUtils.isNotBlank(icebergConfig.get(IcebergConfig.CATALOG_URI)), | ||
| "Catalog uri can't be blank"); | ||
| // The method `get` will call the method `checkValue`, just check the values of the config | ||
| // options |
There was a problem hiding this comment.
It's not proper to add the comment here. It explains the behavior of config::get, unrelated to iceberg here.
There was a problem hiding this comment.
OK. I will remove 'The method get will call the method checkValue'. But it's also confusing if we only call the method get, but we don't use it.
There was a problem hiding this comment.
I got your point. I will change the comment
The method `get` will check the values of config options.
| package com.datastrato.gravitino.config; | ||
|
|
||
| public interface ConfigConstants { | ||
| String NOT_BLANK_ERROR_MSG = "The value can't be the blank"; |
There was a problem hiding this comment.
"The value can't be blank"
| if (validator == null) { | ||
| return; | ||
| } | ||
| optionValue.ifPresent(value -> validator.accept(value)); |
There was a problem hiding this comment.
Can you please optimize the logic here? It is a bit confusing to have both null and optional check here.
There was a problem hiding this comment.
if optionValue is empty, we will skip the check.
There was a problem hiding this comment.
I will add some comments here.
// If optionValue is empty, the config entry will skip the check.
There was a problem hiding this comment.
I think you can change like this:
Stream.of(Optional.ofNullable(validator), optionValue).allMatch(Optional::isPresent) {
validator.accept(value)
}
| package com.datastrato.gravitino.config; | ||
|
|
||
| public interface ConfigConstants { | ||
| String NOT_BLANK_ERROR_MSG = "The value can't be blank"; |
There was a problem hiding this comment.
I think using 'Missing config' can more clearly indicate the error.
There was a problem hiding this comment.
If you configure " ", the validator will report error, too.
| @@ -42,8 +42,7 @@ public class IcebergTableOps implements AutoCloseable { | |||
| public IcebergTableOps(IcebergConfig icebergConfig) { | |||
| String catalogType = icebergConfig.get(IcebergConfig.CATALOG_BACKEND); | |||
| if (!IcebergCatalogBackend.MEMORY.name().equalsIgnoreCase(catalogType)) { | |||
| // The method `get` will call the method `checkValue`, just check the values of the config | |||
| // options | |||
| // The method `get` will check the values of config options. | |||
There was a problem hiding this comment.
You don't have to add the comments here, it's not necessary.
| @@ -45,8 +45,7 @@ private static JdbcCatalog loadJdbcCatalog(Map<String, String> properties) { | |||
| IcebergConfig icebergConfig = new IcebergConfig(properties); | |||
| String driverClassName = icebergConfig.getJdbcDriver(); | |||
|
|
|||
| // The method `get` will call the method `checkValue`, just check the values of the config | |||
| // options | |||
| // The method `get` will check the values of config options. | |||
What changes were proposed in this pull request?
Now, ConfigEntry lacks the ability of check config options. So we add the support of this feature referring to Spark.
Why are the changes needed?
Fix: #1221
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Add new ut.