refactor: remove zookeeper segment announcement and discovery#19377
Conversation
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
The changes LGTM, no correctness issues found.
gianm
left a comment
gianm
left a comment
There was a problem hiding this comment.
👍 on the removal. I think a few too many tests got removed, as mentioned in line comments. Beyond that there's a couple of references that could also be removed:
- in
docs/configuration/logging.md, the example log file mentionsBatchServerInventoryViewwhich doesn't exist anymore - in
MainTestthere is a setting ofdruid.serverview.type = batchin one of the tests. It's expecting an exception to be thrown. I believe it's now passing for a different reason than intended.
| import java.util.concurrent.Executor; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class BrokerServerViewTest extends CuratorTestBase |
There was a problem hiding this comment.
There's coverage of BrokerServerView in this test that we should preserve, such as testing for handling of tiers, etc. It should be reworked to use some other base view.
There was a problem hiding this comment.
oops, restored, thanks for catching
| import java.util.concurrent.ExecutorService; | ||
|
|
||
| @RunWith(Parameterized.class) | ||
| public class CoordinatorServerViewTest extends CuratorTestBase |
There was a problem hiding this comment.
Like BrokerServerView, the tests here should be reworked to use some other base view.
| /** | ||
| * | ||
| */ | ||
| public class BatchDataSegmentAnnouncerTest |
There was a problem hiding this comment.
Some of the removed tests in this file look like they are testing functionality that still exists, so they should be restored. Including:
- testAnnounceSegmentWithSameSegmentConcurrently
- testAnnounceSegmentsWithSameSegmentConcurrently
- testSchemaAnnounce
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
The changes LGTM, no correctness issues found.
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
The changes LGTM, no correctness issues found.
gianm
left a comment
gianm
left a comment
There was a problem hiding this comment.
I know I just approved this but a few other things to consider removing are inline. Beyond that, check docs/api-reference/service-status-api.md, it references "Zookeeper segment discovery" which doesn't happen anymore.
|
|
||
| ``` | ||
| ${druid.zk.paths.liveSegmentsPath}/${druid.host} | ||
| ${druid.zk.paths.announcementsPath}/${druid.host} |
There was a problem hiding this comment.
Is druid.zk.paths.announcementsPath really still used? It looked like this PR removed it.
There was a problem hiding this comment.
oops, yea it can go too
| @JsonProperty | ||
| private String propertiesPath; | ||
| @JsonProperty | ||
| private String announcementsPath; |
There was a problem hiding this comment.
It looks like this isn't used anymore
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 0 |
| P3 | 0 |
| Total | 1 |
Findings that could not be attached inline:
- server/src/main/java/org/apache/druid/server/coordinator/rules/PartialLoadRule.java:94 - [P1] Partial load rules still enqueue full segment loads.
RunRulescallsrule.run(...)afterappliesTo, but this implementation discards the matcher'swrappedLoadSpecandfingerprintand callsreplicateSegmentwith the originalDataSegment. Any configuredloadPartial*rule whose matcher resolves will therefore load the full segment, not the selected projections, so the new public rule types silently violate the partial-load contract until the action path can pass the wrapped load spec through.
This is an automated review by Codex GPT-5
I guess this is a comment for wrong PR? |
3 participants
Description
Druid has carried both ZooKeeper- and HTTP-based paths for segment announcement and inventory for several releases. The ZK paths have been
@Deprecatedfor years, off by default (druid.serverview.type=http is the default), and segment-publishing path, the ZK-based inventory view, and the now-orphanedZkCoordinator/DataSegmentServerAnnouncer/CuratorDataSegmentServerAnnouncer.After this change, ZooKeeper's responsibilities in Druid are limited to:
BatchDataSegmentAnnouncerseems a lot nicer now since it only has to track change history for the lister resource, to something better in a follow-up, but skipped for now.Release note
ZooKeeper-based segment announcement and discovery removed
The ZooKeeper-based segment announcement and inventory view, which have been deprecated and off by default for several releases, have been removed. The HTTP-based path (the default for
druid.serverview.type=http) is now the only supported option.If your configuration sets
druid.serverview.typeto anything other thanhttp, startup will now fail with a clear error message. Remove the property (or set it tohttp, which is the default) to proceed.The following configuration properties are no longer recognized and should be removed from
common.runtime.properties:druid.announcer.segmentsPerNodedruid.announcer.maxBytesPerNodedruid.announcer.skipLoadSpecdruid.announcer.skipDimensionsAndMetricsdruid.announcer.skipSegmentAnnouncementOnZkdruid.zk.paths.announcementsPathdruid.zk.paths.liveSegmentsPathdruid.zk.paths.propertiesPathdruid.zk.paths.connectorPathZooKeeper is still used for leader election, service (node) announcement and discovery, and Overlord-to-MiddleManager task management.