-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ISSUE-631 : Schema Registry numeric properties cannot be set via a property file #629
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @heritamas for raising this PR. Overall LGTM, left a few minor comments.
schema-registry/common/src/main/java/com/hortonworks/registries/schemaregistry/ConfigEntry.java
Outdated
Show resolved
Hide resolved
schema-registry/common/src/main/java/com/hortonworks/registries/schemaregistry/ConfigEntry.java
Outdated
Show resolved
Hide resolved
schema-registry/common/src/main/java/com/hortonworks/registries/schemaregistry/ConfigEntry.java
Outdated
Show resolved
Hide resolved
schema-registry/common/src/main/java/com/hortonworks/registries/schemaregistry/ConfigEntry.java
Outdated
Show resolved
Hide resolved
…erty file During initialization property file is read into a Properties object and validated against a predefined set of rules. However, it was implicitly assumed, that property values can only String typed. This fix adds the necessary conversion beside the already existing validation.
schema-registry/common/src/main/java/com/hortonworks/registries/schemaregistry/ConfigEntry.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
...ent/src/main/java/com/hortonworks/registries/schemaregistry/client/SchemaRegistryClient.java
Show resolved
Hide resolved
schema-registry/common/src/main/java/com/hortonworks/registries/schemaregistry/ConfigEntry.java
Show resolved
Hide resolved
…erty file Specific ConfigTypeConversionException type added to signal different conversion problems. Conversions are made more explicit, errors are signaled via exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 LGTM, thanks @heritamas for the PR. I will merge this PR once the exception class has been updated with the license header.
...ain/java/com/hortonworks/registries/schemaregistry/errors/ConfigTypeConversionException.java
Show resolved
Hide resolved
…erty file License header has been added.
I usually avoid taking extra fields if it can be inferred from other fields. Can we avoid using the @SuppressWarnings("unchecked")
public static <T> T parseValue(Class<T> clazz, Object value) {
if (clazz.equals(String.class)) {
return (T) String.valueOf(value);
} else if (clazz.equals(Integer.class)) {
try {
return (T) Integer.valueOf(value.toString());
} catch (NumberFormatException ex) {
throw new ConfigTypeConversionException(ex.getMessage(), ex);
}
} else if (clazz.equals(Long.class)) {
return (T) Long.valueOf(value.toString());
} else {
throw new ConfigTypeConversionException("Found " + value.getClass() + " instead of " + clazz + " for value: " + value);
}
}
finalValue = ConfigEntry.parseValue(configEntry.type(), value); I'm fine with both approaches. |
Adding a new dependency may not worth it. Let's go with your approach. |
@heritamas and @kamalcph I gonna go ahead and merge this PR. |
During initialization property file is read into a Properties object and
validated against a predefined set of rules. However, it was implicitly
assumed, that property values can only String typed.
This fix adds the necessary conversion beside the already existing validation.
FIxes #631