HDFS-16705. RBF: Support healthMonitor timeout configurable and cache NN and client proxy in NamenodeHeartbeatService #4662
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
can we add some junit tests? |
|
💔 -1 overall
This message was automatically generated. |
|
Thanks @slfan1989 for your review.
Do you want me to add UT for which case? |
updateNameSpaceInfoParameters ? updateSafeModeParameters ? In my personal opinion, if we change the code, we still need to add junit test, and the content of the test should cover the changed part. |
slfan1989
reviewed
Jul 30, 2022
.../src/main/java/org/apache/hadoop/hdfs/server/federation/router/NamenodeHeartbeatService.java
Show resolved
Hide resolved
I just encapsulated them into functions without changing the logic. This can make the code logic clearer and make the reader have a better mood. Of course, if it is necessary to add some UTs about it, I will do it. Anyway, thanks for your review. @slfan1989 |
|
@ZanderXu Thanks for your contribution, pr is very valuable! |
goiri
changed the title
goiri
reviewed
Jul 30, 2022
.../src/main/java/org/apache/hadoop/hdfs/server/federation/router/NamenodeHeartbeatService.java
Outdated
Show resolved
Hide resolved
| @@ -160,35 +167,28 @@ protected void serviceInit(Configuration configuration) throws Exception { | |||
| this.rpcAddress = getRpcAddress(conf, nameserviceId, originalNnId); | |||
|
|
|||
| // Get the Service RPC address for monitoring | |||
| this.serviceAddress = | |||
| DFSUtil.getNamenodeServiceAddr(conf, nameserviceId, originalNnId); | |||
| this.serviceAddress = DFSUtil.getNamenodeServiceAddr(conf, nameserviceId, originalNnId); | |||
There was a problem hiding this comment.
Let's restrict the format changes to PRs for refactor.
It is very hard to review and see what is the actual change otherwise.
Let's undo all those.
| int timeoutMs = conf.getInt( | ||
| DFS_ROUTER_HEALTH_MONITOR_TIMEOUT_MS, | ||
| DFS_ROUTER_HEALTH_MONITOR_TIMEOUT_MS_DEFAULT); | ||
| this.healthMonitorTimeoutMs = Math.max(0, timeoutMs); |
There was a problem hiding this comment.
We may want to log if the value was < 0
.../src/main/java/org/apache/hadoop/hdfs/server/federation/router/NamenodeHeartbeatService.java
Show resolved
Hide resolved
| @@ -96,6 +96,10 @@ public class RBFConfigKeys extends CommonConfigurationKeysPublic { | |||
| FEDERATION_ROUTER_PREFIX + "heartbeat.interval"; | |||
| public static final long DFS_ROUTER_HEARTBEAT_INTERVAL_MS_DEFAULT = | |||
| TimeUnit.SECONDS.toMillis(5); | |||
| public static final String DFS_ROUTER_HEALTH_MONITOR_TIMEOUT_MS = | |||
There was a problem hiding this comment.
Let's use the getTimeDuration style.
There was a problem hiding this comment.
@goiri Thanks for your review. getTimeDuration will return a long value, but proxy timeoutMs need an Integer value. If we use getTimeDuration, we need to cast long to int. If you think it's ok, I will modify it.
There was a problem hiding this comment.
Let's do the getTimeDuration and cast it at the end.
There was a problem hiding this comment.
Copy, sir, I will fix it later.
|
💔 -1 overall
This message was automatically generated. |
slfan1989
reviewed
Jul 31, 2022
| localTarget = null; | ||
| } else { | ||
| // Failed to fetch HA status, ignoring failure | ||
| LOG.error("Cannot fetch HA status for {}: {}", getNamenodeDesc(), e.getMessage(), e); |
There was a problem hiding this comment.
And for logging exceptions, when do we need full stack? What doesn't need full stack? What do you think about this?
There was a problem hiding this comment.
Indeed, we can remove e.getMessage() in this place, because there is already a full stack. @goiri do you have any special considerations?
I personally feel that if we don't know the calling relationship of the exception very well and we need the full stack to located exceptions, or there is a very complex calling relationship, we can output the full stack. If we already know the calling process very well and the calling relationship is clear, we can just out put the message of exceptions, it will be clearer.
|
@goiri @slfan1989 Sir, please help me review this patch, thanks. |
|
💔 -1 overall
This message was automatically generated. |
|
@goiri Sir, please help me reivew the patch again, thanks. |
|
💔 -1 overall
This message was automatically generated. |
goiri
approved these changes
Aug 2, 2022
|
@slfan1989 Sir, about this PR, please help me review it. If you have any questions, please let me know. Thanks |
|
@slfan1989 you still have comments. |
|
@slfan1989 Ping, master, can help me review patch? |
|
It looks good from my side but I'd like to get closure from @slfan1989 |
slfan1989
approved these changes
Aug 12, 2022
LGTM. @ZanderXu Thank you for your contribution, @goiri Thank you for helping to review the code. |
|
Thanks @slfan1989. |
|
Sure. |
…ng NN and client proxy in NamenodeHeartbeatService
|
💔 -1 overall
This message was automatically generated. |
|
@goiri ping, Master, can you help me finally review it and merge it? |
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
…ng NN and client proxy in NamenodeHeartbeatService (apache#4662)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
When I read NamenodeHeartbeatService.class of RBF, I feel that there are somethings we can do for NamenodeHeartbeatService.class.