You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "akpatnam25 (via GitHub)" <gi...@apache.org> on 2025/08/27 04:25:41 UTC
[PR] CELEBORN-2130: Add support for redirection application registration to the leader [celeborn]
akpatnam25 opened a new pull request, #3455:
URL: https://github.com/apache/celeborn/pull/3455
<!--
Thanks for sending a pull request! Here are some tips for you:
- Make sure the PR title start w/ a JIRA ticket, e.g. '[CELEBORN-XXXX] Your PR title ...'.
- Be sure to keep the PR description updated to reflect all changes.
- Please write your PR title to summarize what this PR proposes.
- If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
-Detect if celeborn coordinator is a follower during registration and notify application proactively to reconnect to the right leader.
- Reconstruct MasterNotLeaderException based on the rpc failure stringified exception when .
The goal here is to ensure there are no breaking backwards compatibility changes.
Merging this internally, instead of OSS - given the nontrivial changes that have gone in for C++ support in the Apache main branch. I will create a PR based on this once we have validated it in prod.
### Why are the changes needed?
see above
### Does this PR introduce _any_ user-facing change?
no
### How was this patch tested?
added UTs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2130] Add support for redirection application registration to the leader [celeborn]
Posted by "akpatnam25 (via GitHub)" <gi...@apache.org>.
akpatnam25 commented on code in PR #3455:
URL: https://github.com/apache/celeborn/pull/3455#discussion_r2338630996
##########
common/src/main/java/org/apache/celeborn/common/network/sasl/SecretRegistry.java:
##########
@@ -28,4 +30,9 @@ public interface SecretRegistry {
void register(String appId, String secret);
void unregister(String appId);
+
+ /** Checks if the SecretRegistry can register the application. */
+ default boolean ensureRegistrationAllowed() throws IOException {
Review Comment:
updated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2130] Add support for redirection application registration to the leader [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm closed pull request #3455: [CELEBORN-2130] Add support for redirection application registration to the leader
URL: https://github.com/apache/celeborn/pull/3455
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2130] Add support for redirection application registration to the leader [celeborn]
Posted by "akpatnam25 (via GitHub)" <gi...@apache.org>.
akpatnam25 commented on PR #3455:
URL: https://github.com/apache/celeborn/pull/3455#issuecomment-3283748700
updated. PTAL, thanks @SteNicholas
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2130] Add support for redirection application registration to the leader [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #3455:
URL: https://github.com/apache/celeborn/pull/3455#discussion_r2329259447
##########
common/src/main/java/org/apache/celeborn/common/network/sasl/SecretRegistry.java:
##########
@@ -28,4 +30,12 @@ public interface SecretRegistry {
void register(String appId, String secret);
void unregister(String appId);
+
+ /**
+ * Checks if the SecretRegistry can register the application. If it cant register (for example,
+ * not the current leader when running in HA), then throws an exception
+ */
+ default void ensureRegistrationAllowed() throws IOException {
Review Comment:
Why does not this method return boolean value to represent whether the SecretRegistry can register the application?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2130] Add support for redirection application registration to the leader [celeborn]
Posted by "akpatnam25 (via GitHub)" <gi...@apache.org>.
akpatnam25 commented on code in PR #3455:
URL: https://github.com/apache/celeborn/pull/3455#discussion_r2335675812
##########
master/src/test/java/org/apache/celeborn/service/deploy/master/clustermeta/ha/RatisMasterStatusSystemSuiteJ.java:
##########
@@ -1939,6 +1950,66 @@ public void testHandleReportWorkerDecommission() throws InterruptedException {
Assert.assertEquals(2, STATUSSYSTEM3.availableWorkers.size());
}
+ @Test
+ public void testRedirectToLeaderForRegistration() throws Throwable {
+ CelebornConf celebornConf = new CelebornConf();
+ celebornConf.set("celeborn.shuffle.io.maxRetries", "1");
+ celebornConf.set("celeborn.network.bind.preferIpAddress", "false");
+ TransportConf conf = new TransportConf("shuffle", celebornConf);
+
+ // if server1 is the leader, pick server2, else server1 - so that it is always a follower
+ AbstractMetaManager followerStatusSystem =
+ RATISSERVER1.isLeader() ? STATUSSYSTEM2 : STATUSSYSTEM1;
+
+ SecretRegistry registry =
+ new SecretRegistry() {
+ // Only canRegisterElseReturnFailure is relevant for this test, rest dont matter
+ public String getSecretKey(String appId) {
+ return null;
+ }
+
+ public boolean isRegistered(String appId) {
+ return false;
+ }
+
+ public void register(String appId, String secret) {}
+
+ public void unregister(String appId) {}
+
+ public void ensureRegistrationAllowed() throws IOException {
+ // what MasterSecretRegistryImpl does - except we purposefully pick a follower
+ MasterSecretRegistryImpl.ensureRegistrationAllowedImpl(followerStatusSystem, false);
+ }
+ };
+
+ RegistrationServerBootstrap serverBootstrap = new RegistrationServerBootstrap(conf, registry);
+ RegistrationClientBootstrap clientBootstrap =
+ new RegistrationClientBootstrap(
+ conf, "app", new SaslCredentials("user", "password"), new RegistrationInfo());
+ try {
+ SaslTestBase.authHelper(conf, serverBootstrap, clientBootstrap);
+ fail("Should have redirected to leader");
+ } catch (Exception e) {
+ assertTrue(e.getCause() instanceof MasterNotLeaderException);
+ MasterNotLeaderException notLeaderException = (MasterNotLeaderException) e.getCause();
+
+ HARaftServer leader =
Review Comment:
yes, we already do verify it is leader. We need it to check whether it got redirected to the leader below this line
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2130] Add support for redirection application registration to the leader [celeborn]
Posted by "akpatnam25 (via GitHub)" <gi...@apache.org>.
akpatnam25 commented on code in PR #3455:
URL: https://github.com/apache/celeborn/pull/3455#discussion_r2335657672
##########
common/src/main/java/org/apache/celeborn/common/network/sasl/registration/RegistrationClientBootstrap.java:
##########
@@ -259,4 +267,34 @@ private static Map<PbAuthType, Set<String>> findSupportedSaslMechs(
}
return supportedMechs;
}
+
+ @SuppressWarnings("NonInclusiveLanguage")
+ private static final Pattern MASTER_NOT_LEADER_EXCEPTION_PATTERN =
Review Comment:
I think unfortunately given how the error message is, it has to be like this probably
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2130] Add support for redirection application registration to the leader [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #3455:
URL: https://github.com/apache/celeborn/pull/3455#discussion_r2329263438
##########
master/src/test/java/org/apache/celeborn/service/deploy/master/clustermeta/ha/RatisMasterStatusSystemSuiteJ.java:
##########
@@ -1939,6 +1950,66 @@ public void testHandleReportWorkerDecommission() throws InterruptedException {
Assert.assertEquals(2, STATUSSYSTEM3.availableWorkers.size());
}
+ @Test
+ public void testRedirectToLeaderForRegistration() throws Throwable {
+ CelebornConf celebornConf = new CelebornConf();
+ celebornConf.set("celeborn.shuffle.io.maxRetries", "1");
+ celebornConf.set("celeborn.network.bind.preferIpAddress", "false");
+ TransportConf conf = new TransportConf("shuffle", celebornConf);
+
+ // if server1 is the leader, pick server2, else server1 - so that it is always a follower
+ AbstractMetaManager followerStatusSystem =
+ RATISSERVER1.isLeader() ? STATUSSYSTEM2 : STATUSSYSTEM1;
+
+ SecretRegistry registry =
+ new SecretRegistry() {
+ // Only canRegisterElseReturnFailure is relevant for this test, rest dont matter
+ public String getSecretKey(String appId) {
+ return null;
+ }
+
+ public boolean isRegistered(String appId) {
+ return false;
+ }
+
+ public void register(String appId, String secret) {}
+
+ public void unregister(String appId) {}
+
+ public void ensureRegistrationAllowed() throws IOException {
+ // what MasterSecretRegistryImpl does - except we purposefully pick a follower
+ MasterSecretRegistryImpl.ensureRegistrationAllowedImpl(followerStatusSystem, false);
+ }
+ };
+
+ RegistrationServerBootstrap serverBootstrap = new RegistrationServerBootstrap(conf, registry);
+ RegistrationClientBootstrap clientBootstrap =
+ new RegistrationClientBootstrap(
+ conf, "app", new SaslCredentials("user", "password"), new RegistrationInfo());
+ try {
+ SaslTestBase.authHelper(conf, serverBootstrap, clientBootstrap);
+ fail("Should have redirected to leader");
+ } catch (Exception e) {
+ assertTrue(e.getCause() instanceof MasterNotLeaderException);
+ MasterNotLeaderException notLeaderException = (MasterNotLeaderException) e.getCause();
+
+ HARaftServer leader =
Review Comment:
Does this need to verify whether current node is leader?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CELEBORN-2130: Add support for redirection application registration to the leader [celeborn]
Posted by "akpatnam25 (via GitHub)" <gi...@apache.org>.
akpatnam25 commented on PR #3455:
URL: https://github.com/apache/celeborn/pull/3455#issuecomment-3226694870
FYI this is @mridulm's internal PR that I am just forward porting! :)
PTAL @mridulm @FMX @SteNicholas
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2130] Add support for redirection application registration to the leader [celeborn]
Posted by "akpatnam25 (via GitHub)" <gi...@apache.org>.
akpatnam25 commented on PR #3455:
URL: https://github.com/apache/celeborn/pull/3455#issuecomment-3277851490
> @akpatnam25, thanks for update. Only one minor comment. BTW, could you provide the redirection result in your production environment?
Yes, @SteNicholas this has been in our production env for the past 3 months since June and has been working well for us. +CC @mridulm
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2130] Add support for redirection application registration to the leader [celeborn]
Posted by "akpatnam25 (via GitHub)" <gi...@apache.org>.
akpatnam25 commented on PR #3455:
URL: https://github.com/apache/celeborn/pull/3455#issuecomment-3273722156
@SteNicholas I addressed the comments, PTAL. Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2130] Add support for redirection application registration to the leader [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #3455:
URL: https://github.com/apache/celeborn/pull/3455#discussion_r2340422143
##########
common/src/main/java/org/apache/celeborn/common/network/sasl/SecretRegistry.java:
##########
@@ -28,4 +30,9 @@ public interface SecretRegistry {
void register(String appId, String secret);
void unregister(String appId);
+
+ /** Checks if the SecretRegistry can register the application. */
+ default boolean registrationEnabled() throws IOException {
Review Comment:
Why does this method throw IOException? IMO, it's unnecessary for method which returns boolean value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2130] Add support for redirection application registration to the leader [celeborn]
Posted by "akpatnam25 (via GitHub)" <gi...@apache.org>.
akpatnam25 commented on code in PR #3455:
URL: https://github.com/apache/celeborn/pull/3455#discussion_r2335677763
##########
common/src/main/java/org/apache/celeborn/common/network/sasl/SecretRegistry.java:
##########
@@ -28,4 +30,12 @@ public interface SecretRegistry {
void register(String appId, String secret);
void unregister(String appId);
+
+ /**
+ * Checks if the SecretRegistry can register the application. If it cant register (for example,
+ * not the current leader when running in HA), then throws an exception
+ */
+ default void ensureRegistrationAllowed() throws IOException {
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2130] Add support for redirection application registration to the leader [celeborn]
Posted by "akpatnam25 (via GitHub)" <gi...@apache.org>.
akpatnam25 commented on PR #3455:
URL: https://github.com/apache/celeborn/pull/3455#issuecomment-3254570054
ping on this @mridulm @FMX @SteNicholas
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CELEBORN-2130: Add support for redirection application registration to the leader [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #3455:
URL: https://github.com/apache/celeborn/pull/3455#issuecomment-3226738609
Thanks ! I really should have gotten to this sooner :(
I will let others review it, as this was originally authored by me - dont want to bias the review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2130] Add support for redirection application registration to the leader [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #3455:
URL: https://github.com/apache/celeborn/pull/3455#issuecomment-3322187625
Merged to main branch.
Thanks for working on this @akpatnam25 !
Thanks for the review @SteNicholas :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2130] Add support for redirection application registration to the leader [celeborn]
Posted by "akpatnam25 (via GitHub)" <gi...@apache.org>.
akpatnam25 commented on code in PR #3455:
URL: https://github.com/apache/celeborn/pull/3455#discussion_r2343024665
##########
common/src/main/java/org/apache/celeborn/common/network/sasl/SecretRegistry.java:
##########
@@ -28,4 +30,9 @@ public interface SecretRegistry {
void register(String appId, String secret);
void unregister(String appId);
+
+ /** Checks if the SecretRegistry can register the application. */
+ default boolean registrationEnabled() throws IOException {
Review Comment:
sounds good, updated @SteNicholas
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2130] Add support for redirection application registration to the leader [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #3455:
URL: https://github.com/apache/celeborn/pull/3455#discussion_r2336565143
##########
common/src/main/java/org/apache/celeborn/common/network/sasl/SecretRegistry.java:
##########
@@ -28,4 +30,9 @@ public interface SecretRegistry {
void register(String appId, String secret);
void unregister(String appId);
+
+ /** Checks if the SecretRegistry can register the application. */
+ default boolean ensureRegistrationAllowed() throws IOException {
Review Comment:
```suggestion
default boolean registrationEnabled() {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2130] Add support for redirection application registration to the leader [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #3455:
URL: https://github.com/apache/celeborn/pull/3455#discussion_r2329250628
##########
common/src/main/java/org/apache/celeborn/common/network/sasl/registration/RegistrationClientBootstrap.java:
##########
@@ -259,4 +267,34 @@ private static Map<PbAuthType, Set<String>> findSupportedSaslMechs(
}
return supportedMechs;
}
+
+ @SuppressWarnings("NonInclusiveLanguage")
+ private static final Pattern MASTER_NOT_LEADER_EXCEPTION_PATTERN =
Review Comment:
Could the pattern be simplified?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2130] Add support for redirection application registration to the leader [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #3455:
URL: https://github.com/apache/celeborn/pull/3455#discussion_r2329259447
##########
common/src/main/java/org/apache/celeborn/common/network/sasl/SecretRegistry.java:
##########
@@ -28,4 +30,12 @@ public interface SecretRegistry {
void register(String appId, String secret);
void unregister(String appId);
+
+ /**
+ * Checks if the SecretRegistry can register the application. If it cant register (for example,
+ * not the current leader when running in HA), then throws an exception
+ */
+ default void ensureRegistrationAllowed() throws IOException {
Review Comment:
Why does not this method return boolean value to represent whether the SecretRegistry can register the application? IMO, returning boolean value is better than throwing exception for implementation of `SecretRegistry`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org