[ 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: hdfs-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org