feat(server): report cluster-wide graph readiness through pd - #200
Draft
looksaw2 wants to merge 6 commits into
Draft
feat(server): report cluster-wide graph readiness through pd#200looksaw2 wants to merge 6 commits into
looksaw2 wants to merge 6 commits into
Conversation
In distributed mode a graph is created on one server and the others
converge on it independently, so CreateGraph answers before the graph is
bound everywhere and a client behind a load balancer can reach a server
that still answers "Could not rebind [g]". The window has no bound and
no completion signal.
Each server now reports the status of a graph to pd once it opens it:
LOADING before the backend is opened, READY once the graph is bound to
the gremlin server, FAILED when either step fails. The status is keyed
by the id the server registers with, so the report of one server never
overwrites the report of another.
GET graphspaces/{graphspace}/graphs/{name}/status aggregates what the
servers reported against the servers currently registered for the graph
space, which lets a client wait for a graph to be usable cluster wide.
The aggregate is a readiness gate so it fails closed: it answers READY
only when every registered server reported READY, and the status left
behind by a server that is gone is dropped rather than counted.
Relates to apache#3137
Notify the graph create event last, so that nothing that can still fail runs after the event that reports the graph ready. The event is handled without waiting for it, so a failure after it used to have the request thread report the graph failed while the event thread reported it ready, and which of the two answers stayed was a matter of timing. Keep the status of a server that is missing from the registration until it is old enough to be taken as gone. A registration is refreshed periodically and lapses for a while when a server is merely slow, and dropping the status of such a server left only the servers that were ready, answering ready while it was still loading. A server that reported also stays counted, so it can't be dropped from both sides of the comparison at once. Check the role of the caller on the graph. The other reads of the resource verify it while opening the graph, which this one can't do since it has to answer for a graph that is still loading, so a member of the graph space could read the status of a graph it holds no permission on. Read a single config key rather than listing the graphs of the space to tell whether a graph exists, and keep stack traces out of the log on both paths: clients poll this API in a loop. Relates to apache#3137
Reaching ready by counting let the status of a server that is gone stand in for a registered server that never reported: a replaced server whose status was recent enough to be kept balanced out the server that took its place, and the graph read ready while that server couldn't serve it. Ready now asks that every registered server reported ready. Tell a graph config that can't be read apart from one that isn't there. Both answered not found, so a metadata failure was served to the client as a dropped graph, on an API clients poll and are meant to trust. Relates to apache#3137
Drop what this server reported before it restarted, while it is not registered yet. Its id is stable across a restart, so the status it left behind read as the status of the server that was starting, and a graph it had reported ready was counted ready again before it had opened it. Don't report a graph failed when the attempt that failed is not the one that registered it. Opening a graph another attempt of this server registered first fails, and the graph is served all the same, so the report used to overwrite the status of the attempt that succeeded and stay: this server has the graph and won't open it again. An attempt that registered the graph itself and failed afterwards still reports. Say so when the id of this server falls back to the rest server url. That url is the same for every replica of a container image, so the servers share one id, report their graphs over each other and are counted as one, and nothing said that the status was keyed by an identity that can't tell them apart. Relates to apache#3137
The config directory is read whether or not this server takes its graphs from it, and the name of such a graph comes from a file name, so a local file named after a graph of the cluster reported over the graph of the cluster: ready for one that no other server had opened yet, or failed for a healthy one when the local open failed. A graph of the config directory is known to this server alone, and the status API already answers for it from the local instance rather than from the cluster metadata. Drop a test that asserted nothing: opening a graph this server already has is refused before any status is reported, so the test never reached the case it was written for and passed with the case removed. Relates to apache#3137
an id that isn't unique A graph that failed after it was registered stayed in the map of this server: it was answered for while it reported failed, and this server never opened it again, so the failure was final. Take it back out and close it, so that opening it can be tried again and the report of the attempt that succeeds replaces the one that didn't. Leave a server whose id can't be derived out of the readiness of its graphs. The rest server url is the same for every server started from one image, so the servers that fall back to it are one id to the cluster, and one of them reporting ready used to answer for all of them. Reporting the servers as unknown instead keeps their graphs below ready, which is what the rest of this API does when it can't tell. Relates to apache#3137
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## helm-dev #200 +/- ##
===========================================
Coverage ? 34.67%
Complexity ? 498
===========================================
Files ? 785
Lines ? 67559
Branches ? 9042
===========================================
Hits ? 23423
Misses ? 41504
Partials ? 2632 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
LOADING,READY, orFAILEDfor every PD-backed graphGET /graphspaces/{graphspace}/graphs/{graph}/statuswithready_count,expected_count, and per-Server detailsContext
This is the
helm-devport of apache#3139 for apache#3137. The upstream draft currently conflicts with this branch, so the create path was adapted to preserve the branch's existing rule that a failed local bind must not publish graph metadata.Verification
Related: apache#3137
Based on: apache#3139