refactor: remove zookeeper segment announcement and discovery#19377
refactor: remove zookeeper segment announcement and discovery#19377clintropolis wants to merge 5 commits intoapache:masterfrom
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.
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.liveSegmentsPathdruid.zk.paths.propertiesPathdruid.zk.paths.connectorPathZooKeeper is still used for leader election, service (node) announcement and discovery, and Overlord-to-MiddleManager task management.