[
https://issues.apache.org/jira/browse/HDFS-17892?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18066815#comment-18066815
]
ASF GitHub Bot commented on HDFS-17892:
---------------------------------------
dParikesit commented on code in PR #8345:
URL: https://github.com/apache/hadoop/pull/8345#discussion_r2960672359
##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterRpcClient.java:
##########
@@ -1599,51 +1599,55 @@ protected static <T extends RemoteLocationContext, R>
Map<T, R> postProcessResul
// transfer originCall & callerContext to worker threads of executor.
final Call originCall = Server.getCurCall().get();
final CallerContext originContext = CallerContext.getCurrent();
- for (final T location : locations) {
- String nsId = location.getNameserviceId();
- boolean isObserverRead = isObserverReadEligible(nsId, m);
- final List<? extends FederationNamenodeContext> namenodes =
- getOrderedNamenodes(nsId, isObserverRead);
- final Class<?> proto = method.getProtocol();
- final Object[] paramList = method.getParams(location);
- if (standby) {
- // Call the objectGetter to all NNs (including standby)
- for (final FederationNamenodeContext nn : namenodes) {
- String nnId = nn.getNamenodeId();
- final List<FederationNamenodeContext> nnList =
- Collections.singletonList(nn);
- T nnLocation = location;
- if (location instanceof RemoteLocation) {
- nnLocation = (T)new RemoteLocation(nsId, nnId, location.getDest());
+ try{
+ for (final T location : locations) {
+ String nsId = location.getNameserviceId();
+ boolean isObserverRead = isObserverReadEligible(nsId, m);
+ final List<? extends FederationNamenodeContext> namenodes =
+ getOrderedNamenodes(nsId, isObserverRead);
+ final Class<?> proto = method.getProtocol();
+ final Object[] paramList = method.getParams(location);
+ if (standby) {
+ // Call the objectGetter to all NNs (including standby)
+ for (final FederationNamenodeContext nn : namenodes) {
+ String nnId = nn.getNamenodeId();
+ final List<FederationNamenodeContext> nnList =
+ Collections.singletonList(nn);
+ T nnLocation = location;
+ if (location instanceof RemoteLocation) {
+ nnLocation = (T)new RemoteLocation(nsId, nnId,
location.getDest());
+ }
+ orderedLocations.add(nnLocation);
+ callables.add(
+ () -> {
+ transferThreadLocalContext(originCall, originContext);
+ return invokeMethod(
+ ugi, nnList, isObserverRead, proto, m, paramList);
+ });
}
- orderedLocations.add(nnLocation);
+ } else {
+ // Call the objectGetter in order of nameservices in the NS list
+ orderedLocations.add(location);
callables.add(
() -> {
transferThreadLocalContext(originCall, originContext);
return invokeMethod(
- ugi, nnList, isObserverRead, proto, m, paramList);
+ ugi, namenodes, isObserverRead, proto, m, paramList);
});
}
- } else {
- // Call the objectGetter in order of nameservices in the NS list
- orderedLocations.add(location);
- callables.add(
- () -> {
- transferThreadLocalContext(originCall, originContext);
- return invokeMethod(
- ugi, namenodes, isObserverRead, proto, m, paramList);
- });
}
- }
- if (rpcMonitor != null) {
- rpcMonitor.proxyOp();
- }
- if (this.router.getRouterClientMetrics() != null) {
- this.router.getRouterClientMetrics().incInvokedConcurrent(m);
- }
+ if (rpcMonitor != null) {
+ rpcMonitor.proxyOp();
+ }
+ if (this.router.getRouterClientMetrics() != null) {
+ this.router.getRouterClientMetrics().incInvokedConcurrent(m);
+ }
- return getRemoteResults(method, timeOutMs, controller, orderedLocations,
callables);
+ return getRemoteResults(method, timeOutMs, controller, orderedLocations,
callables);
Review Comment:
Thanks for the feedback! I have moved getRemoteResults outside the try-block
and change finally to catch. Please let me know if you have any other comments.
> RBF: permit not released during invokeConcurrent if getOrderedNamenodes
> throws an exception
> -------------------------------------------------------------------------------------------
>
> Key: HDFS-17892
> URL: https://issues.apache.org/jira/browse/HDFS-17892
> Project: Hadoop HDFS
> Issue Type: Bug
> Components: rbf
> Affects Versions: 3.4.3
> Reporter: Dimas Shidqi Parikesit
> Priority: Critical
> Labels: pull-request-available
>
> After [HDFS-17265|https://issues.apache.org/jira/browse/HDFS-17265], we
> either move acquirePermit after getOrderedNamenodes or wrap
> getOrderedNamenodes in a try-finally block so that the permit can be released
> if getOrderedNamenodes throws an exception.
> However, the behavior of the latest trunk (a178eb7) has regressed, where we
> don't handle the permit release anymore.
> The test failed to detect this problem because it only use 1 locations. In
> invokeConcurrent, the execution flows into invokeSingle where the exception
> is correctly handled using this branch
> ```
> else if (locations.size() == 1 && timeOutMs <= 0) {
> // Shortcut, just one call
> return invokeSingle(locations.iterator().next(), method);
> }
> ```
> In this patch, I've fixed the invokeConcurrent and the test to reflect this
> behavior.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
