Repository navigation
[OBSDATA-15475] Re-land isPodReady service-discovery filter (apache/druid#19139) on Druid 34 - #504
Open
Sadananda Aithal (saithal-confluent) wants to merge 5 commits into
Conversation
…#19139) Reverts 5568688 (#473). The /status/ready endpoint (apache#19148, #495) is now in this branch's ancestry, so any tag containing the filter also contains the endpoint. Do not roll a tag built from this to a cluster that has not completed its Phase B probe-path flip (OBSDATA-15474); the filter plus probes still on /status/selfDiscovered reproduces the 2026-07-20 devel-44409 overlord crash-loop. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Sadananda Aithal (saithal-confluent)
requested a review
from a team
as a code owner
October 15, 2026 10:54
There was a problem hiding this comment.
🟡 Changes recommended
Watch-event handling can undo termination filtering, and discovery behavior, test coverage, and probe guidance need corrections.
5 open findings
What changed in this PR
Reintroduces readiness-based Kubernetes service discovery on Druid 34, building on the existing /status/ready endpoint. Rollout requires verified probe migration on every target cluster.
Changes:
- Filters unready pods and translates readiness changes into discovery events.
- Makes Kubernetes removals idempotent and adds regression tests.
- Documents readiness probes and updates spelling exceptions.
| File | Description |
|---|---|
| website/.spelling | Adds Kubernetes terminology. |
| server/src/test/java/org/apache/druid/discovery/BaseNodeRoleWatcherTest.java | Tests duplicate additions and idempotent removals. |
| server/src/main/java/org/apache/druid/discovery/BaseNodeRoleWatcher.java | Adds skip-if-unknown removal support. |
| extensions-core/kubernetes-extensions/src/test/java/org/apache/druid/k8s/discovery/K8sDruidNodeDiscoveryProviderTest.java | Tests readiness removal and recovery. |
| extensions-core/kubernetes-extensions/src/test/java/org/apache/druid/k8s/discovery/DefaultK8sApiClientTest.java | Tests container readiness evaluation. |
| extensions-core/kubernetes-extensions/src/main/java/org/apache/druid/k8s/discovery/WatchResult.java | Defines readiness-related watch events. |
| extensions-core/kubernetes-extensions/src/main/java/org/apache/druid/k8s/discovery/K8sDruidNodeDiscoveryProvider.java | Handles unready events as idempotent removals. |
| extensions-core/kubernetes-extensions/src/main/java/org/apache/druid/k8s/discovery/DefaultK8sApiClient.java | Filters pods and translates watch events. |
| docs/development/extensions-core/kubernetes.md | Explains readiness-dependent discovery and probes. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+191
to
+192
| if (!isPodReady(item.object)) { | ||
| if (WatchResult.MODIFIED.equals(item.type)) { |
| // Remap MODIFIED (pod ready) events to ADDED for discovery cache purposes. | ||
| // This is safe even if the node is already in the cache because BaseNodeRoleWatcher.childAdded() uses | ||
| // putIfAbsent, so duplicates are silently ignored. | ||
| effectiveType = WatchResult.ADDED; |
Comment on lines
+212
to
+216
| } else if (WatchResult.MODIFIED.equals(item.type)) { | ||
| // Remap MODIFIED (pod ready) events to ADDED for discovery cache purposes. | ||
| // This is safe even if the node is already in the cache because BaseNodeRoleWatcher.childAdded() uses | ||
| // putIfAbsent, so duplicates are silently ignored. | ||
| effectiveType = WatchResult.ADDED; |
(cherry picked from commit 010be96)
…backoff, tests - isPodReady requires the pod Ready condition plus all containers ready, so node-lost pods (stale container statuses) and not-ready sidecars are dropped. - Port the BOOKMARK resourceVersion fix from apache#19056. - Back off watcherErrorRetryWaitMS before relisting on a null watch item. - Add watchPods tests against a mock watch stream and expand isPodReady tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The MODIFIED->ADDED remap re-added stuck-terminating pods that listPods had excluded. Share the termination rule between list and watch: past the check duration, MODIFIED maps to NOT_READY and ADDED is skipped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
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.
Description
Reverts #473 (5568688), re-landing the
isPodReady()filter from apache#19139 inDefaultK8sApiClient/BaseNodeRoleWatcher, with its tests, docs and.spelling.The
/status/readyendpoint (apache#19148, #495) is already an ancestor of this branch, so any image tag built from this also contains the endpoint.ROLLOUT GATE
Ancestry is necessary but not sufficient. The filter deadlocks against the probe path, not against the endpoint's presence. Never roll a tag built from this to a cluster that has not completed its Phase B probe-path flip (OBSDATA-15474). Filter plus probes on
/status/selfDiscoveredreproduces the 2026-07-20 druid-devel-44409 overlord crash-loop. Poison tag: v34.01.32 (filter without endpoint).Rollback: reverting this PR (Phase C) is safe. Reverting Phase B while this is live is forbidden.
Release note
None. The filter has no user-facing config.
Key changed/added classes
DefaultK8sApiClientK8sDruidNodeDiscoveryProviderBaseNodeRoleWatcherThis PR has:
References
kubernetes-extensionsapache/druid#19139; endpoint: Add /status/ready endpoint for service health. apache/druid#19148Testing
BaseNodeRoleWatcherTest(7),DefaultK8sApiClientTest(7),K8sDruidNodeDiscoveryProviderTest(5), all pass on JDK 17. Checkstyle, PMD and forbiddenapis were skipped locally. Not tested in a live cluster.Open Questions
🤖 Generated with Claude Code