HDFS-15757 RBF: Improving Router Connection Management #2651
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
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
goiri
reviewed
Jan 26, 2021
| /** Last timestamp the connection was active. */ | ||
| private long lastActiveTs = 0; | ||
| /** The connection's active status would expire after this window. */ | ||
| private long activeWindow = 30 * 1000; |
There was a problem hiding this comment.
Let's add the unit and probably do: activeWindowMs = TimeUnit.SECONDS.toMillis(30)
...rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/metrics/FederationRPCMetrics.java
Show resolved
Hide resolved
|
@goiri Added tests and addressed some comments. |
|
🎊 +1 overall
This message was automatically generated. |
goiri
approved these changes
Jan 30, 2021
Hexiaoqiao
reviewed
Jan 31, 2021
There was a problem hiding this comment.
Thanks @fengnanli for you works here. Leave some nit comment inline.
Sorry I do not get why the change can reduce connections here after review the changes, is it related "Be greedy here to close as many connections as possible in one shot"? It will be helpful if we add some javadocs explicitly. Thanks.
...dfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/ConnectionContext.java
Outdated
Show resolved
Hide resolved
| } else { | ||
| removed.add(conn); | ||
| if (this.connections.size() > this.minSize) { | ||
| int targetCount = Math.min(num, this.connections.size() - this.minSize); |
There was a problem hiding this comment.
IIRC, connections is not thread safe, so is it possible targetCount is negative here？
There was a problem hiding this comment.
I don't think it can negative here since the only place connections become less is in this function at the swap part with the tmpConnections. The other place where this var gets assigned is in the creation part and it can only increase the value.
...p-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/ConnectionPool.java
Outdated
Show resolved
Hide resolved
goiri
reviewed
Feb 2, 2021
| * was active in the past period of time. | ||
| */ | ||
| public synchronized boolean isActiveRecently() { | ||
| return isActive() || |
There was a problem hiding this comment.
If we do the timestamp based, do we even need to check for the isActive() or the timestamp comparisson is enoug?
There was a problem hiding this comment.
That can removed since the timewindow calculation covers the active case. Updated.
|
🎊 +1 overall
This message was automatically generated. |
Thanks for the review @Hexiaoqiao I put the reason behind this change in the design doc in the original JIRA ticket. In short, I did synchronous connection closing + better picking connections + greedy closing connections. I have seen 50% reduce in number of connections and better ProxyTime. It will be great if you can try in your setup as well. |
|
🎊 +1 overall
This message was automatically generated. |
Hexiaoqiao
approved these changes
Feb 4, 2021
There was a problem hiding this comment.
Thanks @fengnanli for your works and detailed explain. It makes sense to me. +1 from my side.
DazhuangSu
suggested changes
May 8, 2021
| RPC.stopProxy(proxy); | ||
| public synchronized void close(boolean force) throws IllegalStateException { | ||
| if (!force && this.numThreads > 0) { | ||
| throw new IllegalStateException("Active connection cannot be closed"); |
There was a problem hiding this comment.
@fengnanli @Hexiaoqiao
we shouldn't throw a RuntimeException here.
whenever CleanupTask run into this , throwing a runtime exception will cause the threadpool(which is used to close idle connection ) in ConnectionManager to hung.
And no more CleanupTask will be scheduled.
There was a problem hiding this comment.
Good point @DazhuangSu I made the change accordingly to replace the exception with an error log (since there is not much we can do even we catch the exception).
@Hexiaoqiao @goiri Can we consider merging this one since it is there for a long time and there is no issue from some users' experience (except for the this suggestion which will be fixed.)
HDFS-15757 RBF: Improve Router connection management
|
Thanks @fengnanli for your work. |
|
🎊 +1 overall
This message was automatically generated. |
goiri
approved these changes
May 19, 2021
kiran-maturi
pushed a commit
to kiran-maturi/hadoop
that referenced
this pull request
Nov 24, 2021
simbadzina
pushed a commit
to simbadzina/hadoop
that referenced
this pull request
Sep 13, 2022
LiuGuH
pushed a commit
to LiuGuH/hadoop
that referenced
this pull request
Mar 26, 2024
NyteKnight
pushed a commit
to NyteKnight/hadoop
that referenced
this pull request
Jun 25, 2024
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.
NOTICE
Please create an issue in ASF JIRA before opening a pull request,
and you need to set the title of the pull request which starts with
the corresponding JIRA issue number. (e.g. HADOOP-XXXXX. Fix a typo in YYY.)
For more details, please see https://cwiki.apache.org/confluence/display/HADOOP/How+To+Contribute