fix: Restarted the watch after stream reset #19055 by vivek807 · Pull Request #19056 · apache/druid

vivek807 · 2026-02-25T22:54:31Z

Fixes #19055.

Description

Restarted the watch after stream reset

Fixed the bug #19055

This PR has:

Copilot

Pull request overview

Fixes issue #19055 by improving resiliency of the Kubernetes service-discovery watch so broker node inventory can recover after watch stream disruptions.

Changes:

Updated WatchResult to be usable with try-with-resources by extending AutoCloseable.
Refactored NodeRoleWatcher.keepWatching to use try-with-resources and added explicit handling for HTTP/2 stream reset conditions.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
`extensions-core/kubernetes-extensions/src/main/java/org/apache/druid/k8s/discovery/WatchResult.java`	Adjusts the watch iterator contract to support automatic resource management.
`extensions-core/kubernetes-extensions/src/main/java/org/apache/druid/k8s/discovery/K8sDruidNodeDiscoveryProvider.java`	Updates watch loop to auto-close resources and attempts to restart after stream resets/timeouts.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

FrankChen021 · 2026-02-27T08:19:06Z

@vivek807 Thanks for reporting the issue and submitting this PR for fix. can you address above comments?

vivek807 · 2026-03-04T10:56:15Z

@vivek807 Thanks for reporting the issue and submitting this PR for fix. can you address above comments?

updated, please recheck.

capistrant

Still think we need to push further in removing the okhttp internals from this PR. My initial thought is that we just need to handle all IOExceptions with a full re-list. We can keep the one off handling of the known ok IOException that a simple retry of the watch from same resource version will work for. But for all others force a re-list? My biggest fear is that this is an overreaction.

FrankChen021

Follow-up handled: the OkHttp internal StreamResetException dependency was removed, and the watch handling now treats non-interrupt IOExceptions as restartable channel resets while preserving timeout handling. No further inline reply needed from me.

This is an automated review by Codex GPT-5

vivek807 · 2026-06-03T03:31:02Z

@FrankChen021 @gianm @capistrant Please let me know if there’s anything I can do to help move this PR forward.

FrankChen021

I reviewed the update and full PR for correctness, lifecycle, retry, cancellation, cleanup, and concurrency risks; no new issues were found.

Reviewed 5 of 5 changed files.

This is an automated review by Codex GPT-5.6-Sol

AdheipSingh · 2026-08-31T18:01:59Z

@capistrant - Can you please check this.

FrankChen021

Severity	Findings
P0	0
P1	0
P2	1
P3	0
Total	1

I reviewed the full current diff because the prior reviewed baseline SHA was unavailable.

Reviewed 5 of 5 changed files.

This is an automated review by Codex GPT-5.6-Luna(max)

FrankChen021 · 2026-09-04T12:56:48Z

+        }
+        catch (ChannelResetException ex) {
+          LOGGER.debug("Watch stream terminated normally for role[%s], restarting", this.nodeRole);
+          return;


[P2] Persistent resets can cause a tight relist loop

Returning from this handler immediately re-enters the outer watch() loop, which performs listPods() and opens another watch without using watcherErrorRetryWaitMS. If the API server or an intermediary persistently resets every watch stream, this becomes an unbounded list/watch loop that can hammer the Kubernetes API and consume the watcher thread. Apply the configured retry backoff (or otherwise rate-limit full resyncs) before retrying this failure path.

vivek807 · 2026-09-07T07:13:35Z

+                obj = new Watch.Response<>(
+                        WatchResult.BOOKMARK,
+                        new DiscoveryDruidNodeAndResourceVersion(
+                                item.object.getMetadata().getResourceVersion(), null));


BOOKMARK events always has resource version

FrankChen021

I reviewed the incremental update and the full current diff for correctness, edge cases, concurrency, lifecycle, retry, cleanup, compatibility, and data-loss risks; no issues found.

Reviewed 7 of 7 changed files.

This is an automated review by Codex GPT-5.6-Luna(max)

AdheipSingh · 2026-09-22T13:41:47Z

I have manually tested this PR on k8s version 1.35.

vivek807 · 2026-10-08T07:26:14Z

Thanks @FrankChen021 and @AdheipSingh for taking the time to review the changes.

@capistrant I’ve received the required approval. Could you also review the changes and let me know if there’s anything else I can do to help move this PR forward?

FrankChen021 · 2026-10-08T08:08:56Z

@vivek807 can you merge the master to your branch again to see if the CI will be green? recently there're several CI problems when some old PRs were merged into master

Remove hard dependency of okhttp3 internals

Rebasing onto apache/druid master silently duplicated three childRemoved(skipIfUnknown) tests: the JUnit5 versions already in this branch's history merged alongside stale JUnit4 copies from before the junit5-migration commit, instead of conflicting.

Address review comments Signed-off-by: Vivek Dhiman <approach2vivek@gmail.com>

FrankChen021

🟢 Approval recommended

No actionable issues found in this review. Connection resets close the active watch and trigger a full relist after backoff, socket timeouts retain the last processed resource version, and bookmark events carry their own resource version without replaying the previous node event. The current regression tests cover bookmark handling, connection resets, and repeated-reset backoff.

Reviewed 7 of 7 changed files, including the dependency declaration, exception and iterator contracts, both watcher implementations, and both test files. The incremental comparison contains upstream rebase changes; all seven PR files are unchanged from the previous reviewed head. I rechecked the full current diff and surrounding discovery cache and lifecycle code against the latest base.

Validation: git diff --check cde7404de1e6257fce19c9990dd7d94332fca10c HEAD passed. Static review only; no tests or builds were run.

This is an automated review by Codex GPT-5.6-Luna(max)

FrankChen021 added the Kubernetes label Feb 26, 2026

FrankChen021 requested a review from Copilot February 26, 2026 02:04

Copilot started reviewing on behalf of FrankChen021 February 26, 2026 02:04 View session

Copilot AI reviewed Feb 26, 2026

View reviewed changes

vivek807 requested a review from FrankChen021 March 4, 2026 11:00

vivek807 force-pushed the deep/feature/19055-service-discovery-watch-not-recovering-from-stream-reset branch 2 times, most recently from dc86628 to 268b1ff Compare March 4, 2026 12:52

github-actions Bot added the Area - Dependencies label Mar 4, 2026

capistrant requested changes Mar 18, 2026

View reviewed changes

Comment thread .../kubernetes-extensions/src/main/java/org/apache/druid/k8s/discovery/DefaultK8sApiClient.java Outdated

FrankChen021 reviewed Apr 25, 2026

View reviewed changes

Comment thread .../kubernetes-extensions/src/main/java/org/apache/druid/k8s/discovery/DefaultK8sApiClient.java Outdated

FrankChen021 reviewed May 1, 2026

View reviewed changes

vivek807 requested review from FrankChen021, capistrant and gianm May 4, 2026 10:37

vivek807 changed the title ~~Restarted the watch after stream reset #19055~~ fix: Restarted the watch after stream reset #19055 Jun 9, 2026

FrankChen021 reviewed Jul 29, 2026

View reviewed changes

vivek807 force-pushed the deep/feature/19055-service-discovery-watch-not-recovering-from-stream-reset branch from 92fce24 to 9806c18 Compare September 1, 2026 07:05

github-actions Bot added the GHA label Sep 1, 2026

vivek807 force-pushed the deep/feature/19055-service-discovery-watch-not-recovering-from-stream-reset branch 2 times, most recently from 2355d10 to 7298c01 Compare September 1, 2026 07:21

FrankChen021 reviewed Sep 4, 2026

View reviewed changes

vivek807 force-pushed the deep/feature/19055-service-discovery-watch-not-recovering-from-stream-reset branch from a44ac5d to 8559960 Compare September 7, 2026 06:39

github-advanced-security Bot found potential problems Sep 7, 2026

View reviewed changes

FrankChen021 reviewed Sep 7, 2026

View reviewed changes

FrankChen021 approved these changes Sep 22, 2026

View reviewed changes

AdheipSingh approved these changes Sep 22, 2026

View reviewed changes

vivek807 and others added 7 commits October 8, 2026 15:39

Restarted the watch after stream reset

b31a681

Addressed review comments

3df0b36

Add unit test and addresses styling issues

bc2f2d9

Fix service discovery bug in kubernetes-extensions (apache#19139)

8635609

Address review comments.

5b28bc3

Remove hard dependency of okhttp3 internals

Handle Bookmark event

c66ddf8

Address review comments Signed-off-by: Vivek Dhiman <approach2vivek@gmail.com>

vivek807 force-pushed the deep/feature/19055-service-discovery-watch-not-recovering-from-stream-reset branch from 8559960 to c66ddf8 Compare October 8, 2026 10:15

FrankChen021 reviewed Oct 8, 2026

View reviewed changes

FrankChen021 merged commit eb43fd0 into apache:master Oct 8, 2026
27 checks passed

github-actions Bot added this to the 39.0.0 milestone Oct 8, 2026

Conversation

vivek807 commented Feb 25, 2026

Description

Fixed the bug #19055

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

FrankChen021 commented Feb 27, 2026

Uh oh!

vivek807 commented Mar 4, 2026

Uh oh!

capistrant left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

Uh oh!

vivek807 commented Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

Uh oh!

AdheipSingh commented Aug 31, 2026

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

Uh oh!

FrankChen021 Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

vivek807 Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

Uh oh!

AdheipSingh commented Sep 22, 2026

Uh oh!

vivek807 commented Oct 8, 2026

Uh oh!

FrankChen021 commented Oct 8, 2026

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

vivek807 commented Jun 3, 2026 •

edited

Loading