You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/09/26 00:59:25 UTC
[GitHub] [helix] jiajunwang opened a new pull request #1417: Add host name to the Participant History znode.
jiajunwang opened a new pull request #1417:
URL: https://github.com/apache/helix/pull/1417
### Issues
- [X] My PR addresses the following Helix issues and references them in the PR description:
Resolves #1396
### Description
- [X] Here are some details about my PR, including screenshots of any UI changes:
Add hostname to the Participant History znode.
This is to ensure that we can track the participant instance allocation even the live instance node has been removed.
### Tests
- [X] The following tests are written for this issue:
TestInstanceHistory
- [ ] The following is the result of the "mvn test" command on the appropriate module:
Running...
### Documentation (Optional)
- In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Commits
- My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
----------------------------------------------------------------
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
[GitHub] [helix] pkuwm commented on a change in pull request #1417: Add host name to the Participant History znode.
Posted by 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
[GitHub] [helix] alirezazamani commented on a change in pull request #1417: Add host name to the Participant History znode.
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1417:
URL: https://github.com/apache/helix/pull/1417#discussion_r496410901
##########
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:
Good point. Do we keep the history more than 10 entries? If not, maybe hostname is fine.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1417: Add host name to the Participant History znode.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1417:
URL: https://github.com/apache/helix/pull/1417#discussion_r496410049
##########
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:
Makes sense. Modified.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1417: Add host name to the Participant History znode.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1417:
URL: https://github.com/apache/helix/pull/1417#discussion_r496412041
##########
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:
Agreed.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1417: Add host name to the Participant History znode.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1417:
URL: https://github.com/apache/helix/pull/1417#discussion_r496409242
##########
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:
This error indicates the IP of this host cannot be translated to a hostname. This may lead to some connectivity issues. Moreover, the information record in history will be wrong. So I think it is ERROR instead of WARN. If such case happen frequently, then the application admin needs to take a look.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang merged pull request #1417: Add host name to the Participant History znode.
Posted by GitBox <gi...@apache.org>.
jiajunwang merged pull request #1417:
URL: https://github.com/apache/helix/pull/1417
----------------------------------------------------------------
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