Refactor ConfigNode client/info and DirectoryChecker into node-common… - #18561
Refactor ConfigNode client/info and DirectoryChecker into node-common…#18561shizy818 wants to merge 3 commits into
Conversation
8a247d6 to
c67f8af
Compare
| public void invalidate() { | ||
| Optional.ofNullable(transport).ifPresent(TTransport::close); | ||
| protected String getNodeTypeName() { | ||
| return "DataNode"; |
There was a problem hiding this comment.
better define a string constant
| return result; | ||
| @Override | ||
| protected String getNodeTypeName() { | ||
| return "datanode"; |
There was a problem hiding this comment.
reuse the string constant, and why here lower case and previous is upper case.
There was a problem hiding this comment.
Just try to follow original logs:
"Failed to connect to ConfigNode {} from DataNode {} ..."
"Removing is only allowed in an environment where the datanode has been successfully ..."
JackieTien97
left a comment
JackieTien97
left a comment
There was a problem hiding this comment.
Requesting changes because the refactor is not behaviorally equivalent on the existing CGLIB construction path and it breaks the usable ClientManager type. Details are inline.
| * @param <R> the type of rpc result | ||
| * @throws TException if fails more than RETRY_NUM times, throw TException(MSG_RECONNECTION_FAIL) | ||
| */ | ||
| protected <R> R executeRemoteCallWithRetry(final Operation<R> call, final Predicate<R> check) |
There was a problem hiding this comment.
[P1] Preserve the original dispatch semantics under CGLIB
These helpers were private before the refactor, but ConfigNodeClient instances are CGLIB subclasses created by SyncThriftClientWithErrorHandler. Making executeRemoteCallWithRetry (and likewise connectAndSync and updateConfigNodeLeader) non-final protected methods means self-invocations are now intercepted, including during construction. I reproduced this with the CGLIB 3.3.0 version used by the project: getStackTrace()[2] becomes a CGLIB$executeRemoteCallWithRetry$... frame, terminal exceptions are resolved and wrapped twice, and an unchecked exception from updateConfigNodeLeader is converted to TException and retried instead of failing fast. Please keep these internal template methods non-overridable, and make the concrete hook overrides final, or filter the enhancer so that it intercepts only the intended public RPC entry points.
| protected AbstractConfigNodeClient( | ||
| List<TEndPoint> configNodes, | ||
| ThriftClientProperty property, | ||
| ClientManager<ConfigRegionId, ? super AbstractConfigNodeClient> clientManager) |
There was a problem hiding this comment.
[P2] Preserve the concrete client type in ClientManager
ClientManager<ConfigRegionId, ConfigNodeClient>, which was accepted by the old public constructor and is the type used by Factory, is not assignable to ClientManager<ConfigRegionId, ? super AbstractConfigNodeClient>. The current factory works only because reflective construction erases this generic signature; direct callers and future concrete subclasses cannot pass their naturally typed manager. Please retain the concrete self type through the base class, for example AbstractConfigNodeClient<C extends AbstractConfigNodeClient<C>> with ClientManager<ConfigRegionId, C>.
2 participants
…s for reuse
Description
Content1 ...
Content2 ...
Content3 ...
This PR has:
for an unfamiliar reader.
for code coverage.
Key changed/added classes (or packages if there are too many classes) in this PR