[GitHub] [helix] pkuwm commented on a change in pull request #1417: Add host name to the Participant History znode.

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

pkuwm commented on a change in pull request #1417:
URL: https://github.com/apache/helix/pull/1417#discussion_r495424281



##########
File path: helix-core/src/main/java/org/apache/helix/model/ParticipantHistory.java
##########
@@ -47,7 +50,8 @@
     HISTORY,
     OFFLINE,
     VERSION,
-    LAST_OFFLINE_TIME
+    LAST_OFFLINE_TIME,
+    HOST_NAME

Review comment:
       Nit, `HOST` is good enough? As we are writing to zk, I think shorter string could reduce the znode size, since we keep adding the host history to the znode. And I think `HOST` is clear enough.

##########
File path: helix-core/src/test/java/org/apache/helix/integration/paticipant/TestInstanceHistory.java
##########
@@ -44,6 +46,13 @@
     List<String> list = history.getRecord().getListField("HISTORY");
     Assert.assertEquals(list.size(), 1);
 
+    Assert.assertTrue(list.get(0).contains("SESSION=" + _participants[0].getSessionId()));
+    Assert.assertTrue(list.get(0).contains("VERSION=" + _participants[0].getVersion()));
+    Assert
+        .assertTrue(list.get(0).contains("HOST_NAME=" + InetAddress.getLocalHost().getHostName()));

Review comment:
       Nit, this `InetAddress.getLocalHost().getHostName()` doesn't seem to converge to the code. If there is a case this line throws `UnknownHostException`, it makes the test flaky. Do you think we should also this the test converge to the main code?  

##########
File path: helix-core/src/main/java/org/apache/helix/model/ParticipantHistory.java
##########
@@ -76,7 +80,15 @@ public void reportOffline() {
    * @return
    */
   public void reportOnline(String sessionId, String version) {
-    updateSessionHistory(sessionId, version);
+    String hostname;
+    try {
+      hostname = InetAddress.getLocalHost().getHostName();
+    } catch (UnknownHostException e) {
+      LOG.error("Failed to get host name. Use {} for the participant history recording.",

Review comment:
       How about a `WARN` level, since this doesn't break the service? Maybe an error is a bit too much to cause customers to worry about it. I've seen that helix kind of over uses `error` logging that could distract users' attention.




----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org