-
Notifications
You must be signed in to change notification settings - Fork 639
feat(server): add a storage-aware GET /readiness endpoint #3221
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
7167989
fc5ae63
c49de51
b168ced
0a4b846
f42c173
6cdd6ce
067b796
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,7 +53,8 @@ public class LoadDetectFilter implements ContainerRequestFilter { | |
| "", | ||
| "apis", | ||
| "metrics", | ||
| "versions" | ||
| "versions", | ||
| "readiness" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right. New option |
||
| ); | ||
|
|
||
| // Call gc every 30+ seconds if memory is low and request frequently | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with this | ||
| * work for additional information regarding copyright ownership. The ASF | ||
| * licenses this file to You under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance with the | ||
| * License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT | ||
| * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the | ||
| * License for the specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| package org.apache.hugegraph.api.profile; | ||
|
|
||
| import java.util.Map; | ||
|
|
||
| import org.apache.hugegraph.api.API; | ||
| import org.apache.hugegraph.config.HugeConfig; | ||
| import org.apache.hugegraph.config.ServerOptions; | ||
| import org.apache.hugegraph.core.GraphManager; | ||
| import org.apache.hugegraph.util.JsonUtil; | ||
|
|
||
| import com.codahale.metrics.annotation.Timed; | ||
|
|
||
| import io.swagger.v3.oas.annotations.tags.Tag; | ||
| import jakarta.annotation.security.PermitAll; | ||
| import jakarta.inject.Singleton; | ||
| import jakarta.ws.rs.GET; | ||
| import jakarta.ws.rs.Path; | ||
| import jakarta.ws.rs.Produces; | ||
| import jakarta.ws.rs.core.Context; | ||
| import jakarta.ws.rs.core.Response; | ||
|
|
||
| /** | ||
| * Storage-aware readiness for Kubernetes and load balancers: on hstore 200 | ||
| * while at least one known Store answers this server, 503 while none does | ||
| * (or, before any Store list is known, while PD does not answer); on hbase | ||
| * 200 while the cluster answers an admin call within the budget. Unauthenticated, like | ||
| * /versions, so that an httpGet probe needs no credential; the body carries | ||
| * no addresses and no raw exception text. | ||
| */ | ||
| @Path("readiness") | ||
| @Singleton | ||
| @Tag(name = "ReadinessAPI") | ||
| public class ReadinessAPI extends API { | ||
|
|
||
| @GET | ||
| @Timed | ||
| @Produces(APPLICATION_JSON_WITH_CHARSET) | ||
| @PermitAll | ||
| public Response get(@Context GraphManager manager, @Context HugeConfig conf) { | ||
| Map<String, Object> body = StorageReadiness.check( | ||
| manager, conf.get(ServerOptions.READINESS_TIMEOUT), | ||
| conf.get(ServerOptions.READINESS_CACHE_TTL), | ||
| conf.get(ServerOptions.READINESS_MAX_WAITERS)); | ||
| Response.Status status = StorageReadiness.isReady(body) ? | ||
| Response.Status.OK : | ||
| Response.Status.SERVICE_UNAVAILABLE; | ||
| return Response.status(status) | ||
| .type(APPLICATION_JSON_WITH_CHARSET) | ||
| .entity(JsonUtil.toJson(body)) | ||
| .build(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
readinessis added to the auth and path whitelists but not toLoadDetectFilter.WHITE_API_LIST("",apis,metrics,versions). So/readinessgoes through the worker-load and free-memory checks, before the result cache is reached, and gets a 503 oncemax_worker_threads - 1other requests are in flight.restserver.max_worker_threadsdefaults to2 * CPUS, so on a 2-CPU pod the probe fails while 3 other requests run.Under sustained load Kubernetes then drops busy Servers from the Service and shifts their traffic to the rest, so a deployment that moves its readiness probe from
/versionsto/readinesscan lose every endpoint during a spike while storage is fine.Could
readinessgo intoWHITE_API_LISTnext toversions(LoadReleaseFilterreads the same list, so the counter stays balanced), with a case liketestFilter_WhiteListPathIgnored? If shedding load through readiness is intended, please say so in theReadinessAPIJavadoc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 4a4aa1c, thanks, that would have been a nasty interaction: a readiness probe shed under load would pull exactly the busiest Servers out of the Service while the storage is healthy.
readinessis inLoadDetectFilter.WHITE_API_LISTnext toversions;LoadReleaseFilterreads the same list, so theworkLoadcounter stays balanced.testFilter_ReadinessIgnoredLikeVersionsinLoadDetectFilterTest: with a 2-thread limit and one request in flight the filter lets/readinessthrough without touching the counter and without a log entry, in the same shape astestFilter_WhiteListPathIgnored. Readiness is not meant to shed load; theReadinessAPIJavadoc says it answers from the storage state.