Skip to content

add readiness endpoints to processes having initialization delays #8841

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jon-wei merged 1 commit into from
Dec 11, 2019
Merged

add readiness endpoints to processes having initialization delays #8841

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion docs/operations/api-reference.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -752,7 +752,11 @@ Returns segment information lists including server locations for the given datas

* `/druid/broker/v1/loadstatus`

Returns a flag indicating if the Broker knows about all segments in Zookeeper. This can be used to know when a Broker process is ready to be queried after a restart.
Returns a flag indicating if the Broker knows about all segments in the cluster. This can be used to know when a Broker process is ready to be queried after a restart.

* `/druid/broker/v1/readiness`
Copy link
Copy Markdown
Contributor

@fjy fjy Nov 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should have a more general status endpoint instead of having a specific endpoint for 'readiness'. The status endpoint should return a JSON object. Otherwise, this is taking us down a path where we will have API endpoints for everything we want returned from Druid services and things will get messy and complex fast.

Copy link
Copy Markdown
Contributor

@fjy fjy Nov 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think 'readiness' is the incorrect term. I think 'available' is stronger.

Copy link
Copy Markdown

@sascha-coenen sascha-coenen Nov 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

readiness is a better term IMHO because it is an official term for health checking that also has well defined semantics:

https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-startup-probes/

More and more apps are exposing health check endpoints to allow for easy integration into orhestration solutions like Kubernetes

Copy link
Copy Markdown
Member Author

@pjain1 pjain1 Nov 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/readiness already exists for historicals so I just extended it for brokers as well, the main reason for its existence is to know when the node is ready to serve queries as opposed to /status which just tells if process is up or not. Because of this reason I think we need two endpoints - one for checking when the process is ready to serve and another for checking if the process if up or not. Many monitoring systems rely on HTTP response codes to make decisions about service availability/readiness that's why I think /readiness was added even though /loadstatus already existed. BTW for health also we have two endpoints /status and /status/health introduced in #5087.

Copy link
Copy Markdown
Member

@nishantmonu51 nishantmonu51 Nov 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree with @sascha-coenen readiness is a commonly used term for health checks.
One change though would be to have a generic endpoint /readiness instead of node type specific endpoints.
would suggest removing /druid/<node_type>/v1 prefix.

Copy link
Copy Markdown
Member Author

@pjain1 pjain1 Nov 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nishantmonu51 Historical already has /druid/historical/v1/readiness so just extended it for broker - /druid/broker/v1/readiness to be consistent, but can change it to just /readiness if required, no preference from my side.

Copy link
Copy Markdown
Member Author

@pjain1 pjain1 Nov 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fjy I am not very concerned about what should be the name of the endpoint, my opinion is that we need an endpoint that returns meaningful HTTP status code so that integration with monitoring/orchestration solutions becomes easier. So I think either we can expose just /readiness for all node types that have initialization delay or just add /druid/broker/v1/readiness to broker as /druid/historical/v1/readiness already exists for historicals. What do you think ?

Copy link
Copy Markdown
Member

@clintropolis clintropolis Dec 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 'readiness' is the correct term for this, and putting it at /druid/broker/v1/readiness seems acceptable for me since there is precedent with historicals. A universal /readiness I think might take a bigger refactor since there isn't really a universal concept of readiness across all node types, or any sort of health style interface that all node types implement, but is worth considering in the future.


Similar to `/druid/broker/v1/loadstatus`, but instead of returning a JSON, responses 200 OK if its ready and otherwise 503 SERVICE UNAVAILABLE.

#### Queries

Expand Down
13 changes: 12 additions & 1 deletion server/src/main/java/org/apache/druid/server/http/BrokerResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
import javax.ws.rs.core.Response;

@Path("/druid/broker/v1")
@ResourceFilters(StateResourceFilter.class)
public class BrokerResource
{
private final BrokerServerView brokerServerView;
Expand All @@ -45,9 +44,21 @@ public BrokerResource(BrokerServerView brokerServerView)

@GET
@Path("/loadstatus")
@ResourceFilters(StateResourceFilter.class)
@Produces(MediaType.APPLICATION_JSON)
public Response getLoadStatus()
{
return Response.ok(ImmutableMap.of("inventoryInitialized", brokerServerView.isInitialized())).build();
}

@GET
@Path("/readiness")
public Response getReadiness()
{
if (brokerServerView.isInitialized()) {
return Response.ok().build();
} else {
return Response.status(Response.Status.SERVICE_UNAVAILABLE).build();
}
}
}
2 changes: 1 addition & 1 deletion server/src/main/java/org/apache/druid/server/http/HistoricalResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
import javax.ws.rs.core.Response;

@Path("/druid/historical/v1")
@ResourceFilters(StateResourceFilter.class)
public class HistoricalResource
{
private final ZkCoordinator coordinator;
Expand All @@ -47,6 +46,7 @@ public HistoricalResource(

@GET
@Path("/loadstatus")
@ResourceFilters(StateResourceFilter.class)
@Produces(MediaType.APPLICATION_JSON)
public Response getLoadStatus()
{
Expand Down
9 changes: 7 additions & 2 deletions services/src/main/java/org/apache/druid/cli/QueryJettyServerInitializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import com.fasterxml.jackson.databind.ObjectMapper;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
import com.google.inject.Inject;
import com.google.inject.Injector;
import com.google.inject.Key;
Expand All @@ -45,16 +46,20 @@
import org.eclipse.jetty.servlet.ServletContextHandler;
import org.eclipse.jetty.servlet.ServletHolder;

import java.util.Collections;
import java.util.List;
import java.util.Set;

/**
*
*/
public class QueryJettyServerInitializer implements JettyServerInitializer
{
private static final Logger log = new Logger(QueryJettyServerInitializer.class);
private static List<String> UNSECURED_PATHS = Collections.singletonList("/status/health");
private static List<String> UNSECURED_PATHS = Lists.newArrayList(
"/status/health",
"/druid/historical/v1/readiness",
Copy link
Copy Markdown
Contributor

@fjy fjy Nov 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use a consistent scheme here. I'd prefer just /status

"/druid/broker/v1/readiness"
Copy link
Copy Markdown
Contributor

@fjy fjy Nov 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

);

private final List<Handler> extensionHandlers;

Expand Down