add max length configuration for service_name and instance_name #609
Conversation
| if (properties.containsKey(lengthKey)) { | ||
| lengthLimited = Math.max(lengthLimited, Integer.valueOf(properties.getProperty(lengthKey, "0"))); | ||
| } |
There was a problem hiding this comment.
I think max would be a little tricky. As we provided a configuration item, I would prefer to use the configuration value to override static codes as always.
Could you explain why do you code as max?
There was a problem hiding this comment.
The default value is 50, which means it is the suggested value. If the configuration is smaller than it which means most of the service names or instance names will be cut especially in k8s environment.
So we suggest the configuration for the length is better bigger than the default value.
There was a problem hiding this comment.
You are adding a feature to provide customization, I can't see why bigger is acceptable, but smaller isn't.
Your explanation seems doesn't make sense.
All length relative configurations are accepting value length in both ways.
Please update the codes to provide an override capability, rather than within this condition.
There was a problem hiding this comment.
ok, I will change code to prefer user's configuration.
| int lengthLimited = lengthDefine.value(); | ||
| String lengthKey = String.format("%s#length", configKey); | ||
| if (properties.containsKey(lengthKey)) { | ||
| lengthLimited = Integer.valueOf(properties.getProperty(lengthKey, "0")); |
There was a problem hiding this comment.
I think we are better to try catch the Integer#valueOf and system.err if type cast fails. WDYT?
CHANGESlog.