You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "xijiu (via GitHub)" <gi...@apache.org> on 2024/09/19 02:10:41 UTC
[PR] KAFKA-16908: Refactor `QuorumConfig` with `AbstractConfig` [kafka]
xijiu opened a new pull request, #17231:
URL: https://github.com/apache/kafka/pull/17231
As title
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16908: Refactor `QuorumConfig` with `AbstractConfig` [kafka]
Posted by "xijiu (via GitHub)" <gi...@apache.org>.
xijiu commented on code in PR #17231:
URL: https://github.com/apache/kafka/pull/17231#discussion_r1774274519
##########
core/src/main/scala/kafka/server/KafkaConfig.scala:
##########
@@ -231,6 +231,9 @@ class KafkaConfig private(doLog: Boolean, val props: util.Map[_, _])
private val _remoteLogManagerConfig = new RemoteLogManagerConfig(this)
def remoteLogManagerConfig = _remoteLogManagerConfig
+ private val _quorumConfig = new QuorumConfig(this)
+ def quorumConfig = _quorumConfig
Review Comment:
> nit: please declare the type explicitly
Done, I have declared the `QuorumConfig` type, PTAL
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16908: Refactor `QuorumConfig` with `AbstractConfig` [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #17231:
URL: https://github.com/apache/kafka/pull/17231#discussion_r1767203834
##########
raft/src/main/java/org/apache/kafka/raft/QuorumConfig.java:
##########
@@ -47,7 +47,7 @@
* controller should be able to transition from standby to active without reloading all of
* the metadata. The standby is a "hot" standby, not a "cold" one.
*/
-public class QuorumConfig {
+public class QuorumConfig extends AbstractConfig {
Review Comment:
Does it need to extend `AbstractConfig` if it's constructor accepts a `AbstractConfig` already?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16908: Refactor `QuorumConfig` with `AbstractConfig` [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #17231:
URL: https://github.com/apache/kafka/pull/17231#discussion_r1773826306
##########
core/src/main/scala/kafka/server/KafkaConfig.scala:
##########
@@ -231,6 +231,9 @@ class KafkaConfig private(doLog: Boolean, val props: util.Map[_, _])
private val _remoteLogManagerConfig = new RemoteLogManagerConfig(this)
def remoteLogManagerConfig = _remoteLogManagerConfig
+ private val _quorumConfig = new QuorumConfig(this)
+ def quorumConfig = _quorumConfig
Review Comment:
nit: please declare the type explicitly
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16908: Refactor `QuorumConfig` with `AbstractConfig` [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #17231:
URL: https://github.com/apache/kafka/pull/17231#discussion_r1769332465
##########
raft/src/main/java/org/apache/kafka/raft/QuorumConfig.java:
##########
@@ -109,62 +109,34 @@ public class QuorumConfig {
.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;
Review Comment:
Could you please keep those local variables since they are not dynamic?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16908: Refactor `QuorumConfig` with `AbstractConfig` [kafka]
Posted by "xijiu (via GitHub)" <gi...@apache.org>.
xijiu commented on code in PR #17231:
URL: https://github.com/apache/kafka/pull/17231#discussion_r1767827389
##########
raft/src/main/java/org/apache/kafka/raft/QuorumConfig.java:
##########
@@ -47,7 +47,7 @@
* controller should be able to transition from standby to active without reloading all of
* the metadata. The standby is a "hot" standby, not a "cold" one.
*/
-public class QuorumConfig {
+public class QuorumConfig extends AbstractConfig {
Review Comment:
> 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
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16908: Refactor `QuorumConfig` with `AbstractConfig` [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #17231:
URL: https://github.com/apache/kafka/pull/17231
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16908: Refactor `QuorumConfig` with `AbstractConfig` [kafka]
Posted by "xijiu (via GitHub)" <gi...@apache.org>.
xijiu commented on PR #17231:
URL: https://github.com/apache/kafka/pull/17231#issuecomment-2368132282
@chia7712 I have modified the code and remove some getter methods from `KafkaConfig`, PTAL
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org