Skip to content

KAFKA-16908: Refactor QuorumConfig with AbstractConfig#17231

Merged
chia7712 merged 4 commits intoapache:trunkfrom
xijiu:gh-refactor_QuorumConfig
Sep 25, 2024
Merged

KAFKA-16908: Refactor QuorumConfig with AbstractConfig#17231
chia7712 merged 4 commits intoapache:trunkfrom
xijiu:gh-refactor_QuorumConfig

Conversation

@xijiu
Copy link
Copy Markdown
Contributor

@xijiu xijiu commented Sep 19, 2024

As title

Committer Checklist (excluded from commit message)

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

Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@xijiu thanks for this patch!

* the metadata. The standby is a "hot" standby, not a "cold" one.
*/
public class QuorumConfig {
public class QuorumConfig extends AbstractConfig {
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.

Does it need to extend AbstractConfig if it's constructor accepts a AbstractConfig already?

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.

Does it need to extend AbstractConfig if it's constructor accepts a AbstractConfig already?

Indeed, having it extend AbstractConfig is unnecessary. And I have removed it. PTAL

@github-actions github-actions bot added the kraft label Sep 20, 2024
Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@xijiu thanks for this patch. Could you please add a QuorumConfig reference to KafkaConfig and then remove the unused quorum variables from KafkaConfig? quorumElectionTimeoutMs, quorumFetchTimeoutMs, quorumElectionBackoffMs, quorumLingerMs, and quorumRetryBackoffMs?

The above behavior is similar to remoteLogManagerConfig in KafkaConfig

.define(QUORUM_REQUEST_TIMEOUT_MS_CONFIG, INT, DEFAULT_QUORUM_REQUEST_TIMEOUT_MS, null, MEDIUM, QUORUM_REQUEST_TIMEOUT_MS_DOC)
.define(QUORUM_RETRY_BACKOFF_MS_CONFIG, INT, DEFAULT_QUORUM_RETRY_BACKOFF_MS, null, LOW, QUORUM_RETRY_BACKOFF_MS_DOC);

private final int requestTimeoutMs;
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.

Could you please keep those local variables since they are not dynamic?

@xijiu xijiu force-pushed the gh-refactor_QuorumConfig branch from 1148a2c to 93d4567 Compare September 23, 2024 12:43
@github-actions github-actions bot added the core Kafka Broker label Sep 23, 2024
@xijiu
Copy link
Copy Markdown
Contributor Author

xijiu commented Sep 23, 2024

@chia7712 I have modified the code and remove some getter methods from KafkaConfig, PTAL

Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

LGTM

def remoteLogManagerConfig = _remoteLogManagerConfig

private val _quorumConfig = new QuorumConfig(this)
def quorumConfig = _quorumConfig
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.

nit: please declare the type explicitly

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.

nit: please declare the type explicitly

Done, I have declared the QuorumConfig type, PTAL

@chia7712 chia7712 merged commit 2637d12 into apache:trunk Sep 25, 2024
tedyu pushed a commit to tedyu/kafka that referenced this pull request Jan 6, 2025
Reviewers: Chia-Ping Tsai <chia7712@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Kafka Broker kraft

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants