KAFKA-16908: Refactor QuorumConfig with AbstractConfig#16388
KAFKA-16908: Refactor QuorumConfig with AbstractConfig#16388johnnychhsu wants to merge 4 commits intoapache:trunkfrom
Conversation
johnnychhsu
changed the title
OmniaGM
left a comment
OmniaGM
left a comment
There was a problem hiding this comment.
Nice refactor, I just left one suggestion to keep things in line with refactoring KafkaConfig.
|
|
||
| public int requestTimeoutMs() { | ||
| return requestTimeoutMs; | ||
| return getInt(QUORUM_REQUEST_TIMEOUT_MS_CONFIG); |
There was a problem hiding this comment.
If we are defining these getters here can we add QuorumConfig to KafkaConfig in similar way to _remoteLogManagerConfig and delete the quorum getters from there (and maybe some of the validators as well). As this will help keeping KafkaConfig tidier cc: @chia7712
There was a problem hiding this comment.
I'm re-thinking the design about inheritance and composition. _remoteLogManagerConfig has a issue that it can't reflect the updated configs (#16394) and I left a comment (#16394 (comment)) about keeping a KafkaConfig reference instead of copying all props of KafkaConfig.
It seems all configs in QuorumConfig can't be updated dynamically, so it is fine for now. However, I'd like to see more consistent design for those individual config class. Hence, maybe we can take a look at #16394 first, and then this PR can follow the same pattern :)
There was a problem hiding this comment.
just check #16394 and it looks like currently the approach is instead of inheriting, we composite an AbstractConfig as a property of the actual config.
I am wondering if in the future we can put all config related implementation in a module, instead of scattering around in different modules. In this case, take this QuorumConfig for example, we can have a constructor that accept KafkaConfig to init the QuorumConfig.
|
@johnnychhsu Could you please take a look at #16394? It adopts another approach to complete the refactor. |
johnnychhsu
force-pushed
the
KAFKA-16908/refactor_QuorumConfig
branch
from
dbea46c to
cf62fcd
Compare
chia7712
left a comment
chia7712
left a comment
There was a problem hiding this comment.
@johnnychhsu thanks for updated PR
|
|
||
| val apiVersions = new ApiVersions() | ||
| private val raftConfig = new QuorumConfig(config) | ||
| private val raftConfig = new QuorumConfig(props) |
| val quorumRequestTimeoutMs = getInt(QuorumConfig.QUORUM_REQUEST_TIMEOUT_MS_CONFIG) | ||
| val quorumRetryBackoffMs = getInt(QuorumConfig.QUORUM_RETRY_BACKOFF_MS_CONFIG) | ||
| private val _quorumConfig = new QuorumConfig(this) | ||
| def quorumConfig = _quorumConfig |
There was a problem hiding this comment.
Could you please use this to return quorum-related configs and then remove all related getters from KafkaConfig? for example: quorumElectionBackoffMs, quorumFetchTimeoutMs, quorumElectionTimeoutMs, quorumLingerMs and quorumRetryBackoffMs can be removed. quorumRequestTimeoutMs can be removed and its' callers should use QuorumConfig#requestTimeoutMs instead
johnnychhsu
force-pushed
the
KAFKA-16908/refactor_QuorumConfig
branch
from
cf62fcd to
d3c729c
Compare
chia7712
left a comment
chia7712
left a comment
There was a problem hiding this comment.
@johnnychhsu thanks for updated patch. It would be nice to check the build :)
| this.config = config; | ||
| } | ||
|
|
||
| public static ConfigDef configDef() { |
| import java.util.Optional; | ||
| import java.util.OptionalInt; | ||
| import java.util.OptionalLong; | ||
| import java.util.Properties; |
There was a problem hiding this comment.
please remove unnecessary change
| props.put(QuorumConfig.QUORUM_ELECTION_BACKOFF_MAX_MS_CONFIG, ELECTION_JITTER_MS); | ||
| props.put(QuorumConfig.QUORUM_FETCH_TIMEOUT_MS_CONFIG, FETCH_TIMEOUT_MS); | ||
| props.put(QuorumConfig.QUORUM_LINGER_MS_CONFIG, LINGER_MS); | ||
| QuorumConfig quorumConfig = new QuorumConfig(props); |
|
close as this was fixed by #17231 |
3 participants
Jira: https://issues.apache.org/jira/browse/KAFKA-16908
Test
run
./gradlew clean raft:test --tests RaftEventSimulationTestand passedrun
./gradlew clean build -x testand succeedCommitter Checklist (excluded from commit message)