Re: [PR] KAFKA-15853: Move KRAFT configs out of KafkaConfig [kafka]
chia7712 merged PR #15775: URL: https://github.com/apache/kafka/pull/15775 -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KRAFT configs out of KafkaConfig [kafka]
OmniaGM commented on PR #15775: URL: https://github.com/apache/kafka/pull/15775#issuecomment-2075595461 > > > Maybe we should rename RaftConfig to QuorumConfig as all configs in QuorumConfig have prefix QUORUM_ > > > This is good point, maybe this can be a followup PR > > After renaming `RaftConfig` to `QuorumConfig`, `KRaftConfigs` created by this PR is a acceptable naming I think Raised here https://github.com/apache/kafka/pull/15797 -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KRAFT configs out of KafkaConfig [kafka]
chia7712 commented on PR #15775: URL: https://github.com/apache/kafka/pull/15775#issuecomment-2075392820 >> Maybe we should rename RaftConfig to QuorumConfig as all configs in QuorumConfig have prefix QUORUM_ > This is good point, maybe this can be a followup PR After renaming `RaftConfig` to `QuorumConfig`, `KRaftConfigs` created by this PR is a acceptable naming I think -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KRAFT configs out of KafkaConfig [kafka]
OmniaGM commented on PR #15775: URL: https://github.com/apache/kafka/pull/15775#issuecomment-2075357578 > Maybe we should rename RaftConfig to QuorumConfig as all configs in QuorumConfig have prefix QUORUM_ This is good point, maybe this can be a followup PR -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KRAFT configs out of KafkaConfig [kafka]
OmniaGM commented on PR #15775: URL: https://github.com/apache/kafka/pull/15775#issuecomment-2075356299 > It seems org.apache.kafka.raft.KafkaRaftManager needs org.apache.kafka.server.config.KafkaConfig won't happen if the 5 getters are moved, right? Sorry I maybe wan't clear `org.apache.kafka.raft.KafkaRaftManage` this was an example for depend circle if we start to move quorum raft and Kraft code into `raft` module. `KafkaRaftManage` at the moment is in core under `kafka.raft` package and not `raft` module and it need `KafkaConfig` to initialise `org.apache.kafka.raft.RaftConfig` (which is waiting for `AbstractConfig` and not `KafkaConfig` and to access the following getters `nodeId`, `ProcessRoles`, `metadataLogDir`, `quorumRequestTimeoutMs`, `controllerListenerNames`, `saslMechanismControllerProtocol`, `saslInterBrokerHandshakeRequestEnable`. In the future if we decided to move `kafka.raft.KafkaRaftManage` it is most likely going to be moved into server and not raft -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KRAFT configs out of KafkaConfig [kafka]
chia7712 commented on PR #15775: URL: https://github.com/apache/kafka/pull/15775#issuecomment-2075340243 > I don't think we should merge them with RaftConfig as RaftConfig should be separate as it is only for the quorum raft and not KRAFT mode which are a bit different in my option. Maybe we should rename `RaftConfig` to `QuorumConfig` as all configs in `QuorumConfig` have prefix `QUORUM_` -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KRAFT configs out of KafkaConfig [kafka]
chia7712 commented on PR #15775: URL: https://github.com/apache/kafka/pull/15775#issuecomment-2075295917 > KafkaConfig will move out of core and into server, however, for the time being I don't plan to make raft depend on server for now as far as I can see the only case I might need to do so is to use the getters from KafkaConfig but they are only 5 getters as far as I can see so don't worth it. I am planning to just remove these getters from KafkaConfig. It seems `org.apache.kafka.raft.KafkaRaftManager needs org.apache.kafka.server.config.KafkaConfig` won't happen if the 5 getters are moved, right? -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KRAFT configs out of KafkaConfig [kafka]
OmniaGM commented on PR #15775: URL: https://github.com/apache/kafka/pull/15775#issuecomment-2075270699 > > If we start to move related kraft classes to raft module like KafkaRaftManager this will be tricky as now org.apache.kafka.raft.KafkaRaftManager needs org.apache.kafka.server.config.KafkaConfig, org.apache.kafka.server.config.KafkaConfig needs org.apache.kafka.raft.RaftConfig and org.apache.kafka.server.BrokerServer needs org.apache.kafka.raft.KafkaRaftManager > > `org.apache.kafka.server.config.KafkaConfig` -> this means `KafkaConfig` will be moved to server module in the future? If so, is it weird to make raft module depend on server module? Please correct me if I misunderstand anything. KafkaConfig will move out of core and into server, however, for the time being I don't plan to make raft depend on server for now as far as I can see the only case I might need to do so is to use the getters from KafkaConfig but they are only 5 getters as far as I can see so don't worth it. I am planning to just remove these getters from KafkaConfig. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KRAFT configs out of KafkaConfig [kafka]
chia7712 commented on PR #15775: URL: https://github.com/apache/kafka/pull/15775#issuecomment-2075158081 > If we start to move related kraft classes to raft module like KafkaRaftManager this will be tricky as now org.apache.kafka.raft.KafkaRaftManager needs org.apache.kafka.server.config.KafkaConfig, org.apache.kafka.server.config.KafkaConfig needs org.apache.kafka.raft.RaftConfig and org.apache.kafka.server.BrokerServer needs org.apache.kafka.raft.KafkaRaftManager `org.apache.kafka.server.config.KafkaConfig` -> this means `KafkaConfig` will be moved to server module in the future? If so, is it weird to make raft module depend on server module? Please correct me if I misunderstand anything. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KRAFT configs out of KafkaConfig [kafka]
OmniaGM commented on PR #15775: URL: https://github.com/apache/kafka/pull/15775#issuecomment-2075131518 > just curios. Why moving to raft module can cause circle dependencies? Currently raft doesn't depend of core or server but I just fear that mixing quorum raft and KRaft mode related classes in same module might led to situations where we hit this issue when we start to move things more and more out of core. One example I have in mind is `KafkaRaftManager` - `KafkaConfig` depends on `RaftConfig` for raft configs - `KafkaRaftManager` depend on `KafkaConfig` and `RaftConfig` and it is used by `BrokerServer` and `ControllerServer` - If we start to move related kraft classes to raft module like `KafkaRaftManager` this will be tricky as now `org.apache.kafka.raft.KafkaRaftManager` needs `org.apache.kafka.server.config.KafkaConfig`, `org.apache.kafka.server.config.KafkaConfig` needs `org.apache.kafka.raft.RaftConfig` and `org.apache.kafka.server.BrokerServer` needs `org.apache.kafka.raft.KafkaRaftManager` Also one other thing to notice is at the moment `RaftConfig` signature accept `AbstractConfig` instead of `KafkaConfig` and it redefine the getters for all raft configs instead of using `KafkaConfig.quorum*` methods which at the moment aren't used at all except `KafkaConfig.quorumRequestTimeoutMs`. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KRAFT configs out of KafkaConfig [kafka]
chia7712 commented on PR #15775: URL: https://github.com/apache/kafka/pull/15775#issuecomment-2075082750 > Personally I think the first option (which keep the pr as it is) is easier to navigate as we know where all the config used by Kafka Raft server (maybe renaming this to KafkaRaftServerConfigs instead of KRaftConfigs would be better) are and where all the config used by Quorum Raft. WDYT? I'm ok with it. BTW, `KafkaRaftServerConfigs` is a bit verbose to me. Maybe `KRaftServerConfigs` is good enough as `KRaft` is a term in kafka world. > I am concerned that this will create other circle dependencies later when we move the raft package and KafkaRaftServer out of core just curios. Why moving to raft module can cause circle dependencies? It seems raft module depends on only `clients` and `server-common`. I feel `KafkaRaftServer` won't be in either `clients` or `server-common` in the future. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KRAFT configs out of KafkaConfig [kafka]
OmniaGM commented on PR #15775: URL: https://github.com/apache/kafka/pull/15775#issuecomment-2074952186 > > Note: We have already RaftConfig but it seems to contain limited amount of configs that only configure controller raft and shouldn't include configs shared by both broker/controller in KRAFT mode or only for broker in KRAFT mode. > > What about moving `KRaftConfig` to raft module? and we can moving all configs from `RaftConfig` to `KRaftConfig`. With those changes, all raft-related configs are in `KRaftConfig`. `RaftConfig` can be a POJO. We can move it to raft module but 1. I am concerned that this will create other circle dependencies later when we move the raft package and KafkaRaftServer out of core 2. I don't think we should merge them with RaftConfig as RaftConfig should be separate as it is only for the quorum raft and not KRAFT mode which are a bit different in my option. Most of the configs in KRaftConfigs are used by SharedServer, KafkaRaftServer or BrokerLifeCycle. I think we either keep the pr as proposed or maybe we can have in server-common module the following 1. MetadataLogConfigs for anything with prefix `metadata` 2. KafkaRaftServerConfigs for the rest anything 3. Keep RaftServer separated but move it to server-common Personally I think the first option is easier to navigate as we know where all the config used by Kafka Raft server (maybe renaming this to `KafkaRaftServerConfigs` instead of `KRaftConfigs` would be better) are and where all the config used by Quorum Raft. WDYT? -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KRAFT configs out of KafkaConfig [kafka]
chia7712 commented on PR #15775: URL: https://github.com/apache/kafka/pull/15775#issuecomment-2074832899 What about moving `KRaftConfig` to raft module? and we can moving all configs from `RaftConfig` to `KRaftConfig`. With those changes, all raft-related configs are in `KRaftConfig`. `RaftConfig` can be a POJO. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KRAFT configs out of KafkaConfig [kafka]
OmniaGM commented on PR #15775: URL: https://github.com/apache/kafka/pull/15775#issuecomment-2074488094 @chia7712 & @mimaison can you please have a look into this PR? -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
