amaechler opened a new pull request, #20476:
URL: https://github.com/apache/druid/pull/20476
### Description
When a Druid process is killed (for example by SIGKILL after a container
OOM) and restarts on the same host and port within
`druid.zk.service.sessionTimeoutMs`, the ephemeral announcement node of its
previous ZooKeeper session still exists. The new process's announcement
`create` fails with `NODEEXISTS`, and the result was dropped because the create
was fire-and-forget. Recovery relied on the announcer's cache reporting the
stale node's removal once the old session expires.
`PathChildrenCache` (`CHILD_REMOVED`) and `CuratorCache` (`NODE_DELETED`)
only report removals of nodes they have already loaded. If ZooKeeper deletes
the stale node before the cache's first `getData`, no event fires and the
announcement is never re-created. The process keeps running and can still win
leader election, which uses a separate path, but stays invisible to internal
discovery until it restarts. We hit this in production at: a restarted
Coordinator's stale node was deleted about 30ms after it announced itself, and
when that Coordinator later became leader, other services failed with `issued
redirect to unknown URL`. Both `PathChildrenAnnouncer` (the default) and
`NodeAnnouncer` have this window.
Related: #20339, where a discovery node that is removed and re-added right
away, the reinstatement this PR relies on, blocks the Overlord from acquiring
leadership. This PR neither fixes nor causes that issue.
#### Handle `NODEEXISTS` when creating an announcement
`createAnnouncement()` in both announcers now passes a background callback.
On `NODEEXISTS` it sets an exists watch on the path, mirroring Curator's
`PersistentNode`:
- `NONODE`: the stale node is already gone, so the announcement is
re-created right away.
- The node exists: its owning session is logged, and `NodeDeleted`
re-creates the announcement. `NodeDataChanged` re-arms the one-shot watch.
- Any other failure of the create or the exists check is logged as a warning
instead of being dropped.
All re-create paths, including the existing cache listeners and
`PathChildrenAnnouncer`'s reconnect handler, now go through
`reinstateAnnouncement(path)`. It uses the current payload and skips paths that
were unannounced in the meantime; the reconnect handler previously passed a
null payload to the create for such paths. Each announcer shares one watcher
instance, so repeated registrations on a path are deduplicated. When the cache
did see the stale node, both the cache and the watch fire; the second create
gets `NODEEXISTS` on our own node, which is harmless.
Alternatives considered:
- Deleting the existing node when its `ephemeralOwner` is another session:
the payload includes `startTime`, so it never matches the old process's
payload, and two live processes sharing a host and port could keep deleting
each other's announcement.
- Switching to `PersistentNode`: it handles this case, but adopting it means
reworking both announcers (compression, parent cleanup, `update()`), which is
more than this fix needs.
#### Tests
New tests in `PathChildrenAnnouncerTest` and `NodeAnnouncerTest` reproduce
the race deterministically, using a second ZooKeeper session that owns the
stale node:
- `testReinstatesWhenStaleNodeIsRemovedBeforeCacheSeesIt` blocks the
announcer client's event thread, so the stale node is gone before the exists
check runs.
- `testReinstatesWhenWatchedStaleNodeIsRemoved` removes the stale node after
the watch is set while the announcer's executor is blocked, so only the watch
can recover the announcement.
Without the fix all four tests time out; the second one also fails when only
the watcher's `NodeDeleted` branch is disabled. With the fix, each passed 25 of
25 repeated runs.
The existing tests waited for `CuratorListener` `CREATE` events, which
Curator no longer delivers once a create has a callback. They now poll until
the node holds the expected payload and belongs to the announcer's session,
using new helpers in `CuratorTestBase`. Because `KillSession` only expires the
session on the client side, `testSessionKilled` now waits about one session
timeout (about 15 s) for the killed session's nodes to go away; the old latch
passed on the failed `NODEEXISTS` create instead.
#### Release note
Fixed a race in ZooKeeper-based service discovery where a service that
restarted within the ZooKeeper session timeout could stay unregistered until
its next restart.
<hr>
This PR has:
- [x] been self-reviewed.
- [x] using the [concurrency
checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md)
- [x] a release note entry in the PR description.
- [x] added Javadocs for most classes and all non-trivial methods. Linked
related entities via Javadoc links.
- [x] added comments explaining the "why" and the intent of the code
wherever would not be obvious for an unfamiliar reader.
- [x] added unit tests or modified existing tests to cover new code paths,
ensuring the threshold for [code
coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md)
is met.
--
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]
