xyuanlu commented on a change in pull request #1043:
URL: https://github.com/apache/helix/pull/1043#discussion_r443091509
##########
File path: helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java
##########
@@ -150,12 +150,22 @@ public String getDomainAsString() {
*/
public Map<String, String> getDomainAsMap() {
String domain = getDomainAsString();
+ Map<String, String> domainAsMap = new HashMap<>();
if (domain == null || domain.isEmpty()) {
- return Collections.emptyMap();
+ return domainAsMap;
+ }
+
+ String[] pathPairs = domain.trim().split(",");
+ for (String pair : pathPairs) {
+ String[] values = pair.trim().split("=");
+ if (values.length != 2 || values[0].isEmpty() || values[1].isEmpty()) {
+ throw new IllegalArgumentException(
+ String.format("Domain-Value pair %s is not valid.", pair));
+ }
+ domainAsMap.put(values[0], values[1]);
Review comment:
I think if the value or key is null, then this pair shouldn't be count
as a valid input.
Since currently the callers of this function only checks if key exists in
the returned map and uses the corresponding value.
`domainAsMap.getOrDefault(key, "Default_" + key`
So I think the user all assume that all k-v pairs in returned map are valid
(not null).
Thus, I think we should throw exception for empty value. (Original version
already handles no splitter and null key errors).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
