[client] Add retry when get one available tablet serverNode fails #426
Conversation
zhaozijun109
force-pushed
the
develop
branch
from
f116150 to
c2287b3
Compare
| AdminReadOnlyGateway gateway = | ||
| GatewayClientProxy.createGatewayProxy( | ||
| () -> | ||
| getOneAvailableTabletServerNode( | ||
| metadataUpdater.getCluster()), | ||
| rpcClient, | ||
| AdminReadOnlyGateway.class); | ||
|
|
There was a problem hiding this comment.
Unnecessary change. Pls revert this line.
There was a problem hiding this comment.
Ok, I will revert this line.
| @@ -52,6 +55,9 @@ | |||
|
|
|||
| /** Utils for metadata for client. */ | |||
| public class MetadataUtils { | |||
| private static final Logger LOG = LoggerFactory.getLogger(MetadataUtils.class); | |||
|
|
|||
| private static final int MAX_RETRY_TIMES = 5; | |||
There was a problem hiding this comment.
Could this retry time be configurable?
There was a problem hiding this comment.
Yes, I will make it as system configuration
|
@gkatzioura also added a retry mechanism here UPDATE: There is a retry method, but its only available in the test utils |
|
@polyzos Good idea, I will try to make a common util method of retry logic. WDYT? @SteNicholas please left some comments, thank you |
| List<ServerNode> aliveTabletServers = null; | ||
| for (int retryTimes = 0; retryTimes <= MAX_RETRY_TIMES; retryTimes++) { | ||
| aliveTabletServers = cluster.getAliveTabletServerList(); | ||
| if (aliveTabletServers.isEmpty()) { |
There was a problem hiding this comment.
When aliveTabletServers is empty in cluster, the retry doesn't work since the cluster wil stil be empty for aliveTabletServers, we need send metadata request to update cluster`.
There was a problem hiding this comment.
OK，I will adjust it，thank you.
|
@zhaozijun109, @polyzos, IMO, the retry mechanism maybe different from different behavior, therefore it may not need to introduce a common util method of retry logic. |
|
@SteNicholas Thank for your advice, I will re-adjust based on all the comments above |
|
@SteNicholas Indeed, but there might be re-use of some implementations.. my thoughts were that it might be best to have them in a centralized place, to avoid introducing multiple retry implementations across the codebase, because I already saw 3 different cases popping up in different places. Whatever you think works best |
zhaozijun109
force-pushed
the
develop
branch
from
c2287b3 to
fc5a64f
Compare
|
@luoyuxia @SteNicholas Could you please help me review again when you have free time? Thank you. |
| @@ -876,6 +876,13 @@ public class ConfigOptions { | |||
| "Enable metrics for client. When metrics is enabled, the client " | |||
| + "will collect metrics and report by the JMX metrics reporter."); | |||
|
|
|||
| public static final ConfigOption<Integer> CLIENT_GET_TABLET_SERVER_NODE_MAX_RETRY_TIMES = | |||
There was a problem hiding this comment.
Add this option in configuration.md.
|
@zhaozijun109, could you create new issue for this pull request? BTW, could you also add some description for this pull request? |
Purpose
Linked issue: close #xxx
Tests
API and Format
Documentation