[
https://issues.apache.org/jira/browse/HDFS-16283?focusedWorklogId=787345&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-787345
]
ASF GitHub Bot logged work on HDFS-16283:
-----------------------------------------
Author: ASF GitHub Bot
Created on: 02/Jul/22 19:00
Start Date: 02/Jul/22 19:00
Worklog Time Spent: 10m
Work Description: ayushtkn commented on code in PR #4524:
URL: https://github.com/apache/hadoop/pull/4524#discussion_r912390702
##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterRpcClient.java:
##########
@@ -1012,6 +1013,9 @@ public <R extends RemoteLocationContext, T> RemoteResult
invokeSequential(
// Valid result, stop here
@SuppressWarnings("unchecked") R location = (R) loc;
@SuppressWarnings("unchecked") T ret = (T) result;
+ if (ret instanceof LastBlockWithStatus) {
+ ((LastBlockWithStatus) ret).getFileStatus().setNamespace(ns);
+ }
Review Comment:
Is this for append? The No I don't think we should do this for all other
API, should restrict our changes to only Append code.
Check if changing the Append code in ``RouterClientProtocol`` helps:
```
@Override
public LastBlockWithStatus append(String src, final String clientName,
final EnumSetWritable<CreateFlag> flag) throws IOException {
rpcServer.checkOperation(NameNode.OperationCategory.WRITE);
List<RemoteLocation> locations = rpcServer.getLocationsForPath(src,
true);
RemoteMethod method = new RemoteMethod("append",
new Class<?>[] {String.class, String.class, EnumSetWritable.class},
new RemoteParam(), clientName, flag);
RemoteResult result = rpcClient
.invokeSequential(method, locations, LastBlockWithStatus.class,
null);
LastBlockWithStatus lbws = (LastBlockWithStatus) result.getResult();
lbws.getFileStatus().setNamespace(result.getLocation().getNameserviceId());
return lbws;
}
```
##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterRpc.java:
##########
@@ -1450,6 +1452,95 @@ public void testProxyRestoreFailedStorage() throws
Exception {
assertEquals(nnSuccess, routerSuccess);
}
+ @Test
+ public void testRewnewLease() throws Exception {
Review Comment:
This test is has become little big, Can we split the create & append apart
into different tests? Can extract the common stuff into a util method and reuse
##########
hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/ClientProtocol.java:
##########
@@ -759,11 +759,19 @@ SnapshotStatus[] getSnapshotListing(String snapshotRoot)
* the last call to renewLease(), the NameNode assumes the
* client has died.
*
+ * @param namespaces The full Namespace list that the release rpc
Review Comment:
seems typo `release` -> `renewLease`
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeRpcServer.java:
##########
@@ -1174,8 +1174,14 @@ public boolean mkdirs(String src, FsPermission masked,
boolean createParent)
}
@Override // ClientProtocol
- public void renewLease(String clientName) throws IOException {
+ public void renewLease(String clientName, List<String> namespaces)
+ throws IOException {
+ if (namespaces != null && namespaces.size() > 0) {
+ LOG.warn("namespaces({}) should be null or empty "
+ + "on NameNode side, please check it.", namespaces);
Review Comment:
throw Exception here, We don't expect Namespaces here and neither wan't to
silently ignore such an occurrence
##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterClientProtocol.java:
##########
@@ -759,14 +762,49 @@ public boolean mkdirs(String src, FsPermission masked,
boolean createParent)
}
}
+ private Map<String, FederationNamespaceInfo> getAvailableNamespaces()
+ throws IOException {
+ Map<String, FederationNamespaceInfo> allAvailableNamespaces =
+ new HashMap<>();
+ namenodeResolver.getNamespaces().forEach(
+ k -> allAvailableNamespaces.put(k.getNameserviceId(), k));
+ return allAvailableNamespaces;
+ }
+
+ /**
+ * Try to get a list of FederationNamespaceInfo for renewLease RPC.
+ */
+ private List<FederationNamespaceInfo> getRewLeaseNSs(List<String> namespaces)
+ throws IOException {
+ if (namespaces == null || namespaces.isEmpty()) {
+ return new ArrayList<>(namenodeResolver.getNamespaces());
+ }
+ List<FederationNamespaceInfo> result = new ArrayList<>();
+ Map<String, FederationNamespaceInfo> allAvailableNamespaces =
+ getAvailableNamespaces();
Review Comment:
Should have some caching here:
Like:
Initially initialise ``availableNamespace`` and for every call check from
this, if some entry isn't found in the stored/cached ``availableNamespace``, In
that case call ``getAvailableNamespaces()`` and update the value of
``availableNamespace``,
if still we don't find the entry after then we can return all the namespace
what we are doing now
Issue Time Tracking
-------------------
Worklog Id: (was: 787345)
Time Spent: 3h (was: 2h 50m)
> RBF: improve renewLease() to call only a specific NameNode rather than make
> fan-out calls
> -----------------------------------------------------------------------------------------
>
> Key: HDFS-16283
> URL: https://issues.apache.org/jira/browse/HDFS-16283
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: rbf
> Reporter: Aihua Xu
> Assignee: Aihua Xu
> Priority: Major
> Labels: pull-request-available
> Attachments: RBF_ improve renewLease() to call only a specific
> NameNode rather than make fan-out calls.pdf
>
> Time Spent: 3h
> Remaining Estimate: 0h
>
> Currently renewLease() against a router will make fan-out to all the
> NameNodes. Since renewLease() call is so frequent and if one of the NameNodes
> are slow, then eventually the router queues are blocked by all renewLease()
> and cause router degradation.
> We will make a change in the client side to keep track of NameNode Id in
> additional to current fileId so routers understand which NameNodes the client
> is renewing lease against.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
