[GitHub] [druid] zhangyue19921010 commented on a change in pull request #10961: k8s discovery module: fix issue for druid.host being more than 63chars not permitted as k8s resource label value

[GitBox <gi...@apache.org>]

zhangyue19921010 commented on a change in pull request #10961:
URL: https://github.com/apache/druid/pull/10961#discussion_r589916239



##########
File path: extensions-core/kubernetes-extensions/src/main/java/org/apache/druid/k8s/discovery/K8sDruidNodeAnnouncer.java
##########
@@ -237,30 +236,14 @@ private String getPodDefAnnocationPath(String annotation)
     return StringUtils.format("%s/%s", POD_ANNOTATIONS_PATH_PREFIX, annotation);
   }
 
-  private static String encodeHostPort(String hostPort)
+  // a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and
+  // must start and end with an alphanumeric character
+  private static String hashEncodeStringForLabelValue(String str)
   {
-    //K8S requires that label values must match regex (([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?
-    //So, it is essential to replace ':' with '-'
-
-    // it is assumed that hostname does not have ':' in it except for separating host and port
-    Preconditions.checkState(
-        hostPort.indexOf(':') == hostPort.lastIndexOf(':'),
-        "hostname in host:port[%s] has ':' in it", hostPort
-    );
-
-    return hostPort.replace(':', '-');
-  }
-
-  private String replaceLast(String str, char oldChar, char newChar)
-  {
-    char[] chars = str.toCharArray();
-    for (int i = chars.length - 1; i >= 0; i--) {
-      if (chars[i] == oldChar) {
-        chars[i] = newChar;
-        break;
-      }
+    int hash = str.hashCode();
+    if (hash < 0) {
+      hash = -1 * hash;

Review comment:
       nit: Just wondering if this is a best way to solve 63chars limitation using `String.hascode()`. As we know, String.hashCode() isn't unique, but it can't be(https://sigpwned.com/2018/08/10/string-hashcode-is-plenty-unique/). And `hash = -1 * hash` will double the probability of conflict.
   In other words, what happens if there is a conflict? Does the conflict will lead to a wrong SelfDiscovery? If not, I think `String.hascode()` is good enough.




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org