[jira] [Commented] (HBASE-28741) Rpc ConnectionRegistry APIs should have timeout
[ https://issues.apache.org/jira/browse/HBASE-28741?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18074385#comment-18074385 ] Nick Dimiduk commented on HBASE-28741: -- ConnectionRegistryRpcStubHolder.fetchClusterIdAndCreateStubs() can permanently orphan its CompletableFuture. The listener callback's success path (creating the RPC client and stubs) is not wrapped in a try-catch. If RpcClientFactory.createClient() or createStubs() throws, the exception propagates into FutureUtils.addListener's catch-all, which logs "Unexpected error caught when processing CompletableFuture" but cannot complete the future. Neither complete() nor completeExceptionally() is ever called, so every caller up the chain hangs indefinitely. A second path exists: if ClusterIdFetcher construction throws, the future is assigned to addr2StubFuture but the method throws before returning, leaving addr2StubFuture pointing at a zombie future that subsequent getStubs() calls will return. Separately, ClusterIdFetcher creates its RPC channel with rpcTimeout=0. The comment says timeout doesn't matter because it's "only a preamble connection header," but the preamble still requires TCP connect and potentially TLS negotiation, which can hang. Both issues were observed together in production: a certificate authority sidecar returning HTTP 500 caused the RPC client constructor to throw UnsupportedOperationException, orphaning the future and hanging the connection for 300 seconds until the framework killed the thread. (written with AI) > Rpc ConnectionRegistry APIs should have timeout > --- > > Key: HBASE-28741 > URL: https://issues.apache.org/jira/browse/HBASE-28741 > Project: HBase > Issue Type: Improvement >Affects Versions: 2.6.0, 2.4.18, 2.5.10 >Reporter: Viraj Jasani >Assignee: Nick Dimiduk >Priority: Major > > ConnectionRegistry are some of the most basic metadata APIs that determine > how clients can interact with the servers after getting required metadata. > These APIs should timeout quickly if they cannot serve metadata in time. > Similar to HBASE-28428 introducing timeout for Zookeeper ConnectionRegistry > APIs, we should also introduce timeout (same timeout values) for Rpc > ConnectionRegistry APIs as well. RpcConnectionRegistry uses HBase RPC > framework with hedge read fanout mode. > We have two options to introduce timeout: > # Use RetryTimer to keep watch on CompletableFuture and make it complete > exceptionally if timeout is reached (similar proposal as HBASE-28428). > # Introduce separate Rpc timeout config for > AbstractRpcBasedConnectionRegistry as the rpc timeout for generic RPC > operations (hbase.rpc.timeout) could be higher. -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (HBASE-28741) Rpc ConnectionRegistry APIs should have timeout
[ https://issues.apache.org/jira/browse/HBASE-28741?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18074383#comment-18074383 ] Nick Dimiduk commented on HBASE-28741: -- We hit this timeout. Let me prepare a patch. > Rpc ConnectionRegistry APIs should have timeout > --- > > Key: HBASE-28741 > URL: https://issues.apache.org/jira/browse/HBASE-28741 > Project: HBase > Issue Type: Improvement >Affects Versions: 2.6.0, 2.4.18, 2.5.10 >Reporter: Viraj Jasani >Assignee: Nick Dimiduk >Priority: Major > > ConnectionRegistry are some of the most basic metadata APIs that determine > how clients can interact with the servers after getting required metadata. > These APIs should timeout quickly if they cannot serve metadata in time. > Similar to HBASE-28428 introducing timeout for Zookeeper ConnectionRegistry > APIs, we should also introduce timeout (same timeout values) for Rpc > ConnectionRegistry APIs as well. RpcConnectionRegistry uses HBase RPC > framework with hedge read fanout mode. > We have two options to introduce timeout: > # Use RetryTimer to keep watch on CompletableFuture and make it complete > exceptionally if timeout is reached (similar proposal as HBASE-28428). > # Introduce separate Rpc timeout config for > AbstractRpcBasedConnectionRegistry as the rpc timeout for generic RPC > operations (hbase.rpc.timeout) could be higher. -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (HBASE-28741) Rpc ConnectionRegistry APIs should have timeout
[
https://issues.apache.org/jira/browse/HBASE-28741?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17867459#comment-17867459
]
Viraj Jasani commented on HBASE-28741:
--
{quote}If there should be an upper bound, it should not be MAX_VALUE. Please
pick something more reasonable. We actually don't need min() here if it is
always going to choose the right hand parameter.
{quote}
Exactly, either we selectively choose 10s as reasonable timeout for
ConnectionRegistry APIs (like we are already doing with HBASE-28745), or we
replace "hbase.rpc.timeout" with new config
"hbase.rpc.connection.registry.timeout" with default value as 10s.
And as you mentioned, we have too many configs, so even if we replace MAX_VALUE
with 10s as part of Math.min() above, then also we should be good.
> Rpc ConnectionRegistry APIs should have timeout
> ---
>
> Key: HBASE-28741
> URL: https://issues.apache.org/jira/browse/HBASE-28741
> Project: HBase
> Issue Type: Improvement
>Affects Versions: 2.6.0, 2.4.18, 2.5.10
>Reporter: Viraj Jasani
>Priority: Major
>
> ConnectionRegistry are some of the most basic metadata APIs that determine
> how clients can interact with the servers after getting required metadata.
> These APIs should timeout quickly if they cannot serve metadata in time.
> Similar to HBASE-28428 introducing timeout for Zookeeper ConnectionRegistry
> APIs, we should also introduce timeout (same timeout values) for Rpc
> ConnectionRegistry APIs as well. RpcConnectionRegistry uses HBase RPC
> framework with hedge read fanout mode.
> We have two options to introduce timeout:
> # Use RetryTimer to keep watch on CompletableFuture and make it complete
> exceptionally if timeout is reached (similar proposal as HBASE-28428).
> # Introduce separate Rpc timeout config for
> AbstractRpcBasedConnectionRegistry as the rpc timeout for generic RPC
> operations (hbase.rpc.timeout) could be higher.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (HBASE-28741) Rpc ConnectionRegistry APIs should have timeout
[ https://issues.apache.org/jira/browse/HBASE-28741?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17867436#comment-17867436 ] Andrew Kyle Purtell commented on HBASE-28741: - AbstractRpcBasedConnectionRegistry is currently Private . By the way Integer.MAX_VALUE is unacceptable as a default timeout. That is effectively forever so then don't introduce a timeout at all! Somehow this got through earlier code review. Please pick something more reasonable as default. bq. We do want much smaller timeout for metadata registry APIs than any other Rpc timeouts. So then why introduce a separate configuration. We keep increasing the number of configurable things, therefore the total complexity of operating the system. Don't add a new configuration variable unless absolutely necessary. > Rpc ConnectionRegistry APIs should have timeout > --- > > Key: HBASE-28741 > URL: https://issues.apache.org/jira/browse/HBASE-28741 > Project: HBase > Issue Type: Improvement >Affects Versions: 2.6.0, 2.4.18, 2.5.10 >Reporter: Viraj Jasani >Priority: Major > > ConnectionRegistry are some of the most basic metadata APIs that determine > how clients can interact with the servers after getting required metadata. > These APIs should timeout quickly if they cannot serve metadata in time. > Similar to HBASE-28428 introducing timeout for Zookeeper ConnectionRegistry > APIs, we should also introduce timeout (same timeout values) for Rpc > ConnectionRegistry APIs as well. RpcConnectionRegistry uses HBase RPC > framework with hedge read fanout mode. > We have two options to introduce timeout: > # Use RetryTimer to keep watch on CompletableFuture and make it complete > exceptionally if timeout is reached (similar proposal as HBASE-28428). > # Introduce separate Rpc timeout config for > AbstractRpcBasedConnectionRegistry as the rpc timeout for generic RPC > operations (hbase.rpc.timeout) could be higher. -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (HBASE-28741) Rpc ConnectionRegistry APIs should have timeout
[ https://issues.apache.org/jira/browse/HBASE-28741?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17867403#comment-17867403 ] Viraj Jasani commented on HBASE-28741: -- FYI [~divneet18] > Rpc ConnectionRegistry APIs should have timeout > --- > > Key: HBASE-28741 > URL: https://issues.apache.org/jira/browse/HBASE-28741 > Project: HBase > Issue Type: Improvement >Affects Versions: 2.6.0, 2.4.18, 2.5.10 >Reporter: Viraj Jasani >Priority: Major > > ConnectionRegistry are some of the most basic metadata APIs that determine > how clients can interact with the servers after getting required metadata. > These APIs should timeout quickly if they cannot serve metadata in time. > Similar to HBASE-28428 introducing timeout for Zookeeper ConnectionRegistry > APIs, we should also introduce timeout (same timeout values) for Rpc > ConnectionRegistry APIs as well. RpcConnectionRegistry uses HBase RPC > framework with hedge read fanout mode. > We have two options to introduce timeout: > # Use RetryTimer to keep watch on CompletableFuture and make it complete > exceptionally if timeout is reached (similar proposal as HBASE-28428). > # Introduce separate Rpc timeout config for > AbstractRpcBasedConnectionRegistry as the rpc timeout for generic RPC > operations (hbase.rpc.timeout) could be higher. -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (HBASE-28741) Rpc ConnectionRegistry APIs should have timeout
[
https://issues.apache.org/jira/browse/HBASE-28741?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17867402#comment-17867402
]
Viraj Jasani commented on HBASE-28741:
--
{code:java}
protected AbstractRpcBasedConnectionRegistry(Configuration conf,
String hedgedReqsFanoutConfigName, String initialRefreshDelaySecsConfigName,
String refreshIntervalSecsConfigName, String minRefreshIntervalSecsConfigName)
throws IOException {
this.hedgedReadFanOut =
Math.max(1, conf.getInt(hedgedReqsFanoutConfigName,
HEDGED_REQS_FANOUT_DEFAULT));
rpcTimeoutMs = (int) Math.min(Integer.MAX_VALUE,
conf.getLong(HConstants.HBASE_RPC_TIMEOUT_KEY,
HConstants.DEFAULT_HBASE_RPC_TIMEOUT));
// XXX: we pass cluster id as null here since we do not have a cluster id
yet, we have to fetch
// this through the master registry...
// This is a problem as we will use the cluster id to determine the
authentication method
rpcClient = RpcClientFactory.createClient(conf, null);
rpcControllerFactory = RpcControllerFactory.instantiate(conf);
populateStubs(getBootstrapNodes(conf));
// could return null here is refresh interval is less than zero
registryEndpointRefresher =
RegistryEndpointsRefresher.create(conf, initialRefreshDelaySecsConfigName,
refreshIntervalSecsConfigName, minRefreshIntervalSecsConfigName,
this::refreshStubs);
}{code}
[~zhangduo] [~apurtell] For RpcConnectionRegistry based timeout, we need to set
different Rpc timeout value above. One of the proposals is to change config
above for:
{code:java}
rpcTimeoutMs = (int) Math.min(Integer.MAX_VALUE,
conf.getLong(HConstants.HBASE_RPC_TIMEOUT_KEY,
HConstants.DEFAULT_HBASE_RPC_TIMEOUT)); {code}
My question is, is it considered incompatible if we introduce new config for
rpcTimeout here? e.g. "hbase.rpc.connection.registry.timeout". We do want much
smaller timeout for metadata registry APIs than any other Rpc timeouts.
One way to look at this is, connection registry implementations are
IA.LimitedPrivate so maybe config level incompatibility is fine? WDYT?
> Rpc ConnectionRegistry APIs should have timeout
> ---
>
> Key: HBASE-28741
> URL: https://issues.apache.org/jira/browse/HBASE-28741
> Project: HBase
> Issue Type: Improvement
>Affects Versions: 2.6.0, 2.4.18, 2.5.10
>Reporter: Viraj Jasani
>Priority: Major
>
> ConnectionRegistry are some of the most basic metadata APIs that determine
> how clients can interact with the servers after getting required metadata.
> These APIs should timeout quickly if they cannot serve metadata in time.
> Similar to HBASE-28428 introducing timeout for Zookeeper ConnectionRegistry
> APIs, we should also introduce timeout (same timeout values) for Rpc
> ConnectionRegistry APIs as well. RpcConnectionRegistry uses HBase RPC
> framework with hedge read fanout mode.
> We have two options to introduce timeout:
> # Use RetryTimer to keep watch on CompletableFuture and make it complete
> exceptionally if timeout is reached (similar proposal as HBASE-28428).
> # Introduce separate Rpc timeout config for
> AbstractRpcBasedConnectionRegistry as the rpc timeout for generic RPC
> operations (hbase.rpc.timeout) could be higher.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
