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
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
