HDFS-15383. RBF: Add support for router delegation token without watch #2047
Conversation
Summary: This patch is targeting improving router's performance for delegation tokens related operations. It achieves the goal by removing watchers from router on tokens since based on our experience. The huge number of watches inside Zookeeper is degrading Zookeeper's performance pretty hard. The current limit is about 1.2-1.5 million. Specific changes: 1. Explicitly disable the watcher to tokens by not using PathChildrenCache or any curator provided cache at all. 2. Schedule a sync task between router and Zookeeper at a configurable interval to make routers sync with their token information through Zookeeper. 3. For token's change, always make sure to change local cache first instead of depending on callbacks of the watch event when using PathChildrenCache. The above three points are trying to make router token cache behaves as close as possible to the case when the PathChildrenCache is used. The below point handles one corner case. 4. Before token remover(a background thread) removes token from Zookeeper, one router will first make sure this token hasn't been renewed by other peers. This happens only when somehow the sync failed for this token that router local cache doesn't have the corret renewal date (expiry date) Test Plan: 1. Add several unit tests covering all common use cases. 2. Deployed on two machines and performing all tests. 3. Pressure testing: create production scale number of tokens (100k) and monitor the sync latency.
fengnanli
changed the title
|
💔 -1 overall
This message was automatically generated. |
|
|
||
| /** | ||
| * Zookeeper based router delegation token store implementation. | ||
| */ | ||
| public class ZKDelegationTokenSecretManagerImpl extends | ||
| ZKDelegationTokenSecretManager<AbstractDelegationTokenIdentifier> { | ||
|
|
||
| public static final String ZK_DTSM_ROUTER_TOKEN_SYNC_INTERVAL = | ||
| "zk-dt-secret-manager.router.token.sync.interval"; |
There was a problem hiding this comment.
a. It is better to keep the same code style.
public static final String ZK_DTSM_ROUTER_TOKEN_SYNC_INTERVAL =
"zk-dt-secret-manager.router.token.sync.interval";
->
public static final String ZK_DTSM_ROUTER_TOKEN_SYNC_INTERVAL =
ZK_CONF_PREFIX + "router.token.sync.interval";
b. IMO, this is common improvement, it should not aimed at Router only, right?
There was a problem hiding this comment.
Point a) changed.
Point b) this class was brought in as a router delegation support effort and was designed for Router initially, hence I put the router name string inside.
| private final ScheduledExecutorService scheduler = | ||
| Executors.newSingleThreadScheduledExecutor(); | ||
|
|
||
| // Local cache of delegation tokens, used for depracating tokens from |
There was a problem hiding this comment.
depracating -> deprecating?
| throw new IOException(ex); | ||
| } | ||
| // Store data to currentTokenMap | ||
| processTokenAddOrUpdate(data); |
There was a problem hiding this comment.
It looks there are double deserialize operations, it should be better if we can merge them to one IMO.
| // 1) add/update tokens to local cache from zk, which is done through | ||
| // processTokenAddOrUpdate above | ||
| // 2) remove tokens in local cache but not in zk anymore | ||
| for (AbstractDelegationTokenIdentifier ident : currentTokens.keySet()) { |
There was a problem hiding this comment.
After this loop, content of loacalTokenCache is equal to currentTokens right? If that, should we set the currentTokens = localTokenCache directly? Please correct me if wrong.
There was a problem hiding this comment.
localTokenCache is a set for tokenIdent, currentTokens is the map to store the actual TokenInformation. We will need to construct all of the TokenInformation to do the swap. And the swap needs to be done in a synchronized block. I think the current way is simpler and following the logic inside its parent class.
| @@ -0,0 +1,216 @@ | |||
| package org.apache.hadoop.hdfs.server.federation.security.token; | |||
There was a problem hiding this comment.
lack of license annotation？
There was a problem hiding this comment.
Sigh.. somehow I missed it. Thanks for pointing out.
goiri
changed the title
|
hi~ @fengnanli Are you still here? I think it's a good work and I hope to see it's done. Or someone can take over this work, e.g me? |
|
@Cosss7 Thanks for pinging me. I was busy with some other stuff and will start addressing comments soon. |
|
@fengnanli Very brilliant work. Expect work to be done. ❤️ |
|
Thanks for the review @Hexiaoqiao ! Have you tested it on your setup? |
|
💔 -1 overall
This message was automatically generated. |
|
It looks good to me. |
|
Thanks @fengnanli for your continued works. LGTM, I would like to give +1 from my side. |
|
Thanks @Hexiaoqiao for the feedback. |
|
Thanks @goiri @Hexiaoqiao @Cosss7 for the review! |
|
@fengnanli @goiri I agree that we can push this improvement ahead and file another JIRA for other modules. |
…ithout watch (apache#2047) Improving router's performance for delegation tokens related operations. It achieves the goal by removing watchers from router on tokens since based on our experience. The huge number of watches inside Zookeeper is degrading Zookeeper's performance pretty hard. The current limit is about 1.2-1.5 million.
apache#2047) Improving router's performance for delegation tokens related operations. It achieves the goal by removing watchers from router on tokens since based on our experience. The huge number of watches inside Zookeeper is degrading Zookeeper's performance pretty hard. The current limit is about 1.2-1.5 million.
apache#2047) Improving router's performance for delegation tokens related operations. It achieves the goal by removing watchers from router on tokens since based on our experience. The huge number of watches inside Zookeeper is degrading Zookeeper's performance pretty hard. The current limit is about 1.2-1.5 million.
* HDFS-15383. RBF: Add support for router delegation token without watch (#2047) Improving router's performance for delegation tokens related operations. It achieves the goal by removing watchers from router on tokens since based on our experience. The huge number of watches inside Zookeeper is degrading Zookeeper's performance pretty hard. The current limit is about 1.2-1.5 million. * HADOOP-17835. Use CuratorCache implementation instead of PathChildrenCache / TreeCache (#3266) Signed-off-by: Akira Ajisaka <aajisaka@apache.org> Co-authored-by: lfengnan <lfengnan@uber.com> Co-authored-by: Viraj Jasani <vjasani@apache.org> Co-authored-by: Melissa You <myou@myou-mn1.linkedin.biz>
Summary:
This patch is targeting improving router's performance
for delegation tokens related operations.
It achieves the goal by removing watchers from router on
tokens since based on our experience. The huge number of
watches inside Zookeeper is degrading Zookeeper's performance
pretty hard. The current limit is about 1.2-1.5 million.
Specific changes:
PathChildrenCache or any curator provided cache at all.
a configurable interval to make routers sync with their
token information through Zookeeper.
first instead of depending on callbacks of the watch event
when using PathChildrenCache.
The above three points are trying to make router token cache
behaves as close as possible to the case when the PathChildrenCache
is used. The below point handles one corner case.
Zookeeper, one router will first make sure this token hasn't
been renewed by other peers. This happens only when somehow the
sync failed for this token that router local cache doesn't have
the corret renewal date (expiry date)
Test Plan:
and monitor the sync latency.
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