bitflicker64 commented on PR #532: URL: https://github.com/apache/hugegraph-doc/pull/532#issuecomment-6517012958
The earlier review comments below still apply at ee4b747; this head does not fix them. - ⚠️ Important: Blocking: no. Summary: This restart guarantee is unqualified for older PD images, but some images that predate `/v1/ready` do not serve that endpoint; an unknown-path request can return HTTP 200 with an Unauthorized body, which satisfies Kubernetes' status-only `httpGet` probe and will not restart a terminal PD. Please limit this sentence to images that include the endpoint and identify the supported image boundary. Evidence: The merged #3358 API-reference note documents the unknown-path 200 response and warns that a status-only probe can treat it as ready. (https://github.com/apache/hugegraph-doc/pull/532#discussion_r4327977001) - Minor: On this Helm page, "set `HG_PD_AUTH_SECRET_KEY` in Store" reads as a manual step, but the chart in apache/hugegraph#3362 (head ebdc8386) already does it. `store-statefulset.yaml` adds `HG_PD_AUTH_SECRET_KEY` from `secretKeyRef` `hugegraph.pd.authSecretName` / `hugegraph.pd.authSecretKey`, the same Secret and key that `pd-statefulset.yaml` gives PD, and `credential_contract_test.yaml` asserts it for `pd.auth.existingSecret` and `pd.auth.key`. The only way to follow this sentence with the chart is `store.extraEnv`, which renders after the chart's entry, so a mismatched value silently replaces the chart's Secret reference and the shutdown report is refused (Store then falls back to lease expiry). The open review on #3362 also asks for this name to become reserved in `store.extraEnv`, which would make the step fail to render. (https://github.com/apache/hugegraph-doc/pull/532#discussion_r4322108183) - ⚠️ Important: Blocking: no. Summary: This is a shared privileged secret, not a per-Store identity: any Store able to read it can invoke `ReportStoreShutdown` with another Store ID. Please state that scope and restrict secret access to trusted internal Store workloads. Evidence: #3362 `StoreShutdownAuthentication` gates on the shared credential, while `PDService.reportStoreShutdown` forwards the request's Store ID directly to `StoreNodeService.reportStoreShutdown`. (https://github.com/apache/hugegraph-doc/pull/532#discussion_r4327977014) - ⚠️ Important: Blocking: no. Summary: A transport failure can occur after PD has persisted the Store as Offline but before Store receives the response, so an RPC error does not prove PD retained the prior record. Please qualify this as the outcome only when PD did not process the report; for an ambiguous result, advise checking the PD record. Evidence: #3362 `PDService` calls `StoreNodeService.reportStoreShutdown` before sending the response, and that method persists the Offline state before returning. (https://github.com/apache/hugegraph-doc/pull/532#discussion_r4327977022) -- 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]
