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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
