Skip to content

MINOR: Drop enable.metadata.quorum config#9934

Merged
hachikuji merged 6 commits intoapache:trunkfrom
hachikuji:drop-enable-metadata-flag
Jan 21, 2021
Merged

MINOR: Drop enable.metadata.quorum config#9934
hachikuji merged 6 commits intoapache:trunkfrom
hachikuji:drop-enable-metadata-flag

Conversation

@hachikuji
Copy link
Copy Markdown
Contributor

The primary purpose of this patch is to remove the internal enable.metadata.quorum configuration. Instead, we rely on process.roles to determine if the self-managed quorum has been enabled. As a part of this, I've done the following:

  1. Replace the notion of "disabled" APIs with "controller-only" APIs. We previously marked some APIs which were intended only for the KIP-500 as "disabled" so that they would not be unintentionally exposed. For example, the Raft quorum APIs were disabled. Marking them as "controller-only" carries the same effect, but makes the intent that they should be only exposed by the KIP-500 controller clearer.
  2. Make ForwardingManager optional in KafkaServer and KafkaApis. Previously we used null if forwarding was enabled and relied on the metadata quorum check.
  3. Make zookeeper.connect an optional configuration if process.roles is defined.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@hachikuji hachikuji force-pushed the drop-enable-metadata-flag branch from 75f4a87 to 4bebbdf Compare January 20, 2021 00:23
@abbccdda
Copy link
Copy Markdown

@hachikuji Do we still need this PR since Colin already did one?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we call it "Early Access" instead of Alpha?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm going to keep this as internal for now. Let's reconsider this after we have something that can actually start up.

@cmccabe
Copy link
Copy Markdown
Contributor

cmccabe commented Jan 20, 2021

@hachikuji Do we still need this PR since Colin already did one?

That PR was for the KIP-500 branch, not for trunk. It also didn't refactor the "disabled" APIs concept, which I think is useful.

Replace the notion of "disabled" APIs with "controller-only" APIs. We previously marked some APIs which were intended only for the KIP-500 as "disabled" so that they would not be unintentionally exposed

That seems reasonable. I guess the idea is that the broker will not include those APIs in its ApiVersionsResponse. However, the controller always will (not that clients will talk directly to the controller anyway...)

@hachikuji
Copy link
Copy Markdown
Contributor Author

@cmccabe Yeah, that's right. Let me see if I can improve this a little bit to make the intent clearer. We have some APIs which will be exposed by both the broker and the controller, so I think this should probably be a set rather than a flag.

@hachikuji
Copy link
Copy Markdown
Contributor Author

I decided to leave the controller-only flag as it is. I think there are further improvements here to make the scope of the API clearer, but the implications for compatibility are subtle enough that we should consider it separately.

Comment thread clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java Outdated
Comment thread raft/README.md Outdated
Comment thread core/src/main/scala/kafka/server/KafkaConfig.scala Outdated
Comment thread core/src/main/scala/kafka/Kafka.scala Outdated
@hachikuji hachikuji force-pushed the drop-enable-metadata-flag branch from 5c75787 to 50fa8e9 Compare January 21, 2021 19:45
Copy link
Copy Markdown

@abbccdda abbccdda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@hachikuji hachikuji merged commit 9689a31 into apache:trunk Jan 21, 2021
ijuma added a commit to ijuma/kafka that referenced this pull request Jan 26, 2021
…e-allocations-lz4

* apache-github/trunk: (562 commits)
  MINOR: remove unused code from MessageTest (apache#9961)
  MINOR: Fix visibility of Log.{unflushedMessages, addSegment} methods (apache#9966)
  KAFKA-12229: Restore original class loader in integration tests using EmbeddedConnectCluster during shutdown  (apache#9942)
  KAFKA-12190: Fix setting of file permissions on non-POSIX filesystems (apache#9947)
  MINOR: Remove `toStruct` and `fromStruct` methods from generated protocol classes (apache#9960)
  MINOR: Fix typo in Utils#toPositive (apache#9943)
  MINOR: MessageUtil: remove some deadcode (apache#9931)
  MINOR: Update zstd-jni to 1.4.8-2 (apache#9957)
  MINOR: Revert assertion in MockProducerTest (apache#9956)
  MINOR: Optimize assertions in unit tests (apache#9955)
  MINOR: Tag `RaftEventSimulationTest` as `integration` and tweak it (apache#9925)
  MINOR: Update to Gradle 6.8.1 (apache#9953)
  MINOR: A few small group coordinator cleanups (apache#9952)
  MINOR: Upgrade ducktape to version 0.8.1  (apache#9933)
  MINOR: fix record time in test shouldWipeOutStandbyStateDirectoryIfCheckpointIsMissing (apache#9948)
  MINOR: Restore interrupt status when closing (apache#9863)
  KAFKA-10357: Extract setup of repartition topics from Streams partition assignor (apache#9848)
  KAFKA-12212; Bump Metadata API version to remove `ClusterAuthorizedOperations` fields (KIP-700) (apache#9945)
  MINOR: log 2min processing summary of StreamThread loop (apache#9941)
  MINOR: Drop enable.metadata.quorum config (apache#9934)
  ...
@ijuma ijuma added the kraft label Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants