abhishekagarwal87 commented on code in PR #14775:
URL: https://github.com/apache/druid/pull/14775#discussion_r1286598970
##########
server/src/main/java/org/apache/druid/discovery/DruidLeaderClient.java:
##########
@@ -256,15 +257,21 @@ public String findCurrentLeader()
);
}
- private String getCurrentKnownLeader(final boolean cached) throws IOException
+ private String getCurrentKnownLeader(final boolean cached)
{
final String leader = currentKnownLeader.accumulateAndGet(
null,
(current, given) -> current == null || !cached ? pickOneHost() :
current
);
if (leader == null) {
- throw new IOE("No known server");
+ throw DruidException.forPersona(DruidException.Persona.ADMIN)
+ .ofCategory(DruidException.Category.NOT_FOUND)
+ .build(
+ "No leader server found for NodeRole [%s] and
leaderRequestPath [%s].",
Review Comment:
`NodeRole` is a technical internal term. Instead, you can do something like
below
```
"A leader node could not be found for [%s] service. check the logs of [%s]
service to validate that service is healthy. ", nodeRole, nodeRole
```
##########
server/src/main/java/org/apache/druid/discovery/DruidLeaderClient.java:
##########
@@ -256,15 +257,21 @@ public String findCurrentLeader()
);
}
- private String getCurrentKnownLeader(final boolean cached) throws IOException
+ private String getCurrentKnownLeader(final boolean cached)
{
final String leader = currentKnownLeader.accumulateAndGet(
null,
(current, given) -> current == null || !cached ? pickOneHost() :
current
);
if (leader == null) {
- throw new IOE("No known server");
+ throw DruidException.forPersona(DruidException.Persona.ADMIN)
+ .ofCategory(DruidException.Category.NOT_FOUND)
Review Comment:
it should be `RUNTIME_FAILURE` and not `NOT_FOUND`.
##########
server/src/main/java/org/apache/druid/discovery/DruidLeaderClient.java:
##########
@@ -256,15 +257,21 @@ public String findCurrentLeader()
);
}
- private String getCurrentKnownLeader(final boolean cached) throws IOException
Review Comment:
I am a bit hesitant of changing the type of exception here. Since callers
might be catching this right now. Maybe keep it IOException for now and just
change the error message.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
