Hi Satish, Thanks for the review. I've updated the KIP to move ZK based solution to the appendix section.
> One minor comment I have is to change the config name from "remote.log.copy.disable" to "remote.log.copy.enable" with the default value being true. I also think the original name "remote.log.copy.disable" makes more sense because users are "disabling" the remote copy feature. Thank you. Luke On Thu, Aug 8, 2024 at 12:21 PM Kamal Chandraprakash < kamal.chandraprak...@gmail.com> wrote: > Hi Satish, > > Thanks for the review! > > > One minor comment I have is to change the config name from > "remote.log.copy.disable" to "remote.log.copy.enable" with the default > value being true. > > I'm inclined towards maintaining the config as "remote.log.copy.disable" > to keep the default value as "false". > > -- > Kamal > > On Thu, Aug 8, 2024 at 9:23 AM Satish Duggana <satish.dugg...@gmail.com> > wrote: > > > Thanks Kamal, and Luke for improving the earlier solution for KRaft. > > > > One minor comment I have is to change the config name from > > "remote.log.copy.disable" to "remote.log.copy.enable" with the default > > value being true. > > > > The solution summary to disable tiered storage on a topic: > > > > - When a user wants to disable tiered storage on a topic, we should > > make sure that local.log and log.retention are same. This is to make > > sure the user understands the implications of the local storage > > requirements while disabling tiered storage and set them > > appropriately. > > > > - Stop copying the log segments to remote storage as broker needs to > > accumulate the required data locally to serve the required data from > > local storage before we disable in remote storage. This will be done > > by updating the config "remote.log.copy.enable" as false. > > > > - We added a guardrail to make sure user understands that disabling > > tiered storage will delete the remote storage data. This is by setting > > "remote.log.delete.on.disable" should be true before setting > > "remote.storage.enable" as false. > > > > > > I think it is better to refactor the KIP to have only the updated > > KRaft based solution and move the ZK based solution to the appendix > > for reference. wdyt? > > > > ~Satish. > > > > > > > > On Wed, 7 Aug 2024 at 17:38, Luke Chen <show...@gmail.com> wrote: > > > > > > Hi all, > > > > > > Based on the original design: > > > When tiered storage is disabled or becomes read-only on a topic, the > > local > > > retention configuration becomes irrelevant, and all data expiration > > follows > > > the topic-wide retention configuration exclusively. > > > > > > That works well. But we are afraid users will not check the document > and > > > "thought" the local log is bound to the local.retention.ms/bytes after > > > `remote.log.copy.disable=true` (i.e. read-only remote storage). The > > > confusion might cause the local disk to be full and bring down the > > broker. > > > To avoid this "surprise" to users, we'd like to add one more validation > > > when `remote.log.copy.disable` is set to true: > > > - validation: when `remote.log.copy.disable=true`, > > > -- `local.retention.ms` must equal to `retention.ms` or -2 (which > > > means `retention.ms` value to be used) > > > -- `local.retention.bytes` must equal to `retention.byes` or -2 > > (which > > > means `retention.ms` value to be used) > > > > > > So, basically, we don't change the original design, just want to make > > sure > > > users are aware of the retention policy change after disabling remote > log > > > copy. > > > > > > Let me know if you have any comments. > > > > > > Thank you. > > > Luke > > > > > > On Fri, Jul 26, 2024 at 8:17 PM Luke Chen <show...@gmail.com> wrote: > > > > > > > Thanks Kamal for the comments. > > > > KIP updated. > > > > > > > > Thanks. > > > > Luke > > > > > > > > On Fri, Jul 26, 2024 at 6:56 PM Kamal Chandraprakash < > > > > kamal.chandraprak...@gmail.com> wrote: > > > > > > > >> Luke, > > > >> > > > >> Thanks for confirming the topic config change validation on the > > controller > > > >> and updating the KIP. > > > >> The updated KIP LGTM. > > > >> > > > >> 1. Can we update the below sentence in the KIP to clarify that > > > >> remote.storage.enable should be true during graceful disablement? > > > >> > > > >> > Users set the configuration > > > >> "remote.storage.enable=false,remote.log.delete.on.disable=true", or > > > >> "remote.copy.disabled=true" for the desired topic, indicating the > > > >> disablement of tiered storage. > > > >> to > > > >> > Users set the configuration > > > >> "remote.storage.enable=false,remote.log.delete.on.disable=true", or > > > >> "remote.storage.enable=true,remote.copy.disabled=true" for the > desired > > > >> topic, indicating the disablement of tiered storage. > > > >> > > > >> 2. Can we clarify in the public interface that the StopReplica v5, > > > >> tiered_epoch, and tiered_state changes are required only for ZK mode > > and > > > >> won't be implemented? > > > >> > > > >> Thanks, > > > >> Kamal > > > >> > > > >> On Fri, Jul 26, 2024 at 1:40 PM Luke Chen <show...@gmail.com> > wrote: > > > >> > > > >> > Hi Kamal, > > > >> > > > > >> > Thanks for the comments. > > > >> > > > > >> > For this: > > > >> > > If we throw an exception from the server for invalid config, > then > > > >> there > > > >> > will be inconsistency between the CLI tools and the actual state > of > > the > > > >> > topic in the cluster. This can cause some confusion to the users > > whether > > > >> > tiered storage is disabled or not. I don't know how the Kraft > topic > > > >> config > > > >> > propagation/validation works. > > > >> > > > > >> > I've confirmed we can validate the topic configuration change on > the > > > >> > controller level, by comparing existing configuration and new > > changed > > > >> > configuration. > > > >> > In my local POC, we can fail the configuration change if it's > > invalid > > > >> like > > > >> > this: > > > >> > > > > >> > # Disable with remote.log.delete.on.disable=false (default) > > > >> > bin/kafka-configs.sh --bootstrap-server {bootstrap-string} \ > > > >> > --alter --entity-type topics --entity-name {topic-name} \ > > > >> > --add-config 'remote.storage.enable=false' > > > >> > > > > >> > Error while executing config command with args '--bootstrap-server > > > >> > {bootstrap-string} --entity-type topics --entity-name {topic-name} > > > >> --alter > > > >> > --add-config remote.storage.enable=false' > > > >> > java.util.concurrent.ExecutionException: > > > >> > org.apache.kafka.common.errors.InvalidConfigurationException: It > is > > > >> invalid > > > >> > to disable remote storage without deleting remote data. If you > want > > to > > > >> keep > > > >> > the remote data, but turn to read only, please set > > > >> `remote.copy.disabled= > > > >> > true`. If you want to disable remote storage and delete all remote > > data, > > > >> > please set > > > >> `remote.storage.enable=false,remote.log.delete.on.disable=true`. > > > >> > > > > >> > I've updated the KIP. Please take a look when available. > > > >> > > > > >> > > > > >> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-950%3A++Tiered+Storage+Disablement > > > >> > > > > >> > Thank you. > > > >> > Luke > > > >> > > > > >> > > > > >> > On Fri, Jul 26, 2024 at 2:05 AM Kamal Chandraprakash < > > > >> > kamal.chandraprak...@gmail.com> wrote: > > > >> > > > > >> > > Correction: > > > >> > > > > > >> > > (2): Wait for all the remote segments to be deleted async due to > > > >> breach > > > >> > by > > > >> > > retention time (or) size, > > > >> > > then set the `remote.storage.enable = false` and > > > >> > > `remote.log.delete.on.disable = true`. This step is optional. > > > >> > > > > > >> > > On Thu, Jul 25, 2024 at 11:13 PM Kamal Chandraprakash < > > > >> > > kamal.chandraprak...@gmail.com> wrote: > > > >> > > > > > >> > > > Hi Chia-Ping, > > > >> > > > > > > >> > > > Thanks for the review! > > > >> > > > > > > >> > > > > If so, what is the purpose of > > > >> `remote.log.delete.on.disable=false`? > > > >> > > > > > > >> > > > IIUC, the purpose of `remote.log.delete.on.disable` is to get > > > >> explicit > > > >> > > > confirmation from the user > > > >> > > > before deleting the remote log segments. The concern raised in > > the > > > >> > thread > > > >> > > > is that if the user > > > >> > > > accidentally changes the value of `remote.storage.enable` from > > true > > > >> to > > > >> > > > false, then remote segments > > > >> > > > get lost. > > > >> > > > > > > >> > > > For ungraceful disablement, (ie) disabling the remote storage > > for > > > >> the > > > >> > > > topic and deleting all the > > > >> > > > remote segments, the user should set both the configs at once: > > > >> > > > > > > >> > > > (1) remote.storage.enable = false and > > remote.log.delete.on.disable = > > > >> > true > > > >> > > > > > > >> > > > If the user accidentally sets only the remote.storage.enable = > > true > > > >> and > > > >> > > > leaves the `remote.log.delete.on.disable` > > > >> > > > with default value of `false`, then we will throw > > ConfigException to > > > >> > > > prevent the deletion of remote logs. > > > >> > > > > > > >> > > > For graceful disablement, the user should set: > > > >> > > > > > > >> > > > (1): remote.copy.disabled = true. > > > >> > > > (2): Wait for all the remote segments to be deleted async due > to > > > >> breach > > > >> > > by > > > >> > > > retention time (or) size, > > > >> > > > then set the `remote.storage.enable = false`. This step > > is > > > >> > > > optional. > > > >> > > > > > > >> > > > Luke, > > > >> > > > > > > >> > > > In ZK mode, once the topic config value gets updated, then it > > gets > > > >> > saved > > > >> > > > in the /configs/topics/<topic> znode. > > > >> > > > If we throw an exception from the server for invalid config, > > then > > > >> there > > > >> > > > will be inconsistency between the CLI tools > > > >> > > > and the actual state of the topic in the cluster. This can > cause > > > >> some > > > >> > > > confusion to the users whether tiered storage > > > >> > > > is disabled or not. I don't know how the Kraft topic config > > > >> > > > propagation/validation works. > > > >> > > > > > > >> > > > -- > > > >> > > > Kamal > > > >> > > > > > > >> > > > On Thu, Jul 25, 2024 at 7:10 PM Chia-Ping Tsai < > > chia7...@gmail.com> > > > >> > > wrote: > > > >> > > > > > > >> > > >> remote.storage.enable=false > > > >> > > >> remote.log.delete.on.disable=false (default) > > > >> > > >> If the topic config is set to this, or changed to this, we'll > > > >> return > > > >> > > >> ConfigException during validation. > > > >> > > >> > > > >> > > >> Pardon me, I'm a bit confused. > > > >> > > >> > > > >> > > >> when `remote.storage.enable=true`, > > > >> > `remote.log.delete.on.disable=false` > > > >> > > is > > > >> > > >> no-op > > > >> > > >> when `remote.storage.enable=false`, > > > >> > `remote.log.delete.on.disable=false` > > > >> > > >> is > > > >> > > >> error > > > >> > > >> > > > >> > > >> If `remote.log.delete.on.disable` must be true when setting > > > >> > > >> `remote.storage.enable` > > > >> > > >> to false, does it mean changing `remote.storage.enable` to > > false is > > > >> > > >> expected to delete remote storage topic data"? > > > >> > > >> > > > >> > > >> If so, what is the purpose of > > > >> `remote.log.delete.on.disable=false`? > > > >> > > >> > > > >> > > >> Best, > > > >> > > >> Chia-Ping > > > >> > > >> > > > >> > > >> Luke Chen <show...@gmail.com> 於 2024年7月25日 週四 下午8:51寫道: > > > >> > > >> > > > >> > > >> > Hi Christo, > > > >> > > >> > > > > >> > > >> > Thanks for your reply. > > > >> > > >> > > > > >> > > >> > > keep the remote.log.disable.policy, but only allow it to > > take a > > > >> > > value > > > >> > > >> of > > > >> > > >> > "delete". > > > >> > > >> > > > > >> > > >> > I agree, or maybe make it a boolean value, and rename it to > > > >> > > >> > `remote.log.delete.on.disable`, which is clearer. > > > >> > > >> > And because of this new config, there will be a case that > the > > > >> config > > > >> > > is > > > >> > > >> > like this: > > > >> > > >> > > > > >> > > >> > remote.storage.enable=false > > > >> > > >> > remote.log.delete.on.disable=false (default) > > > >> > > >> > > > > >> > > >> > That means, in this case, we'll keep all remote storage > > data, but > > > >> > > close > > > >> > > >> all > > > >> > > >> > remote log tasks, and make "log start offset = local log > > start > > > >> > > offset". > > > >> > > >> > This will make the remote storage metadata in an unknown > > state > > > >> > because > > > >> > > >> the > > > >> > > >> > data in the remote storage is inaccessible anymore (since > log > > > >> start > > > >> > > >> moved > > > >> > > >> > to LLSO). And once this topic re-enables the > > > >> > `remote.storage.enable`, > > > >> > > >> the > > > >> > > >> > old remote log metadata will be included, but log start > > offset is > > > >> > not > > > >> > > >> > expected anymore.... > > > >> > > >> > > > > >> > > >> > So, I'd like to propose that we don't allow this > > configuration: > > > >> > > >> > > > > >> > > >> > remote.storage.enable=false > > > >> > > >> > remote.log.delete.on.disable=false (default) > > > >> > > >> > > > > >> > > >> > If the topic config is set to this, or changed to this, > we'll > > > >> return > > > >> > > >> > ConfigException during validation. > > > >> > > >> > > > > >> > > >> > To make it clear, this is the new proposed solution: > > > >> > > >> > > > > >> > > >> > > > > >> > > >> > > > >> > > > > > >> > > > > >> > > > https://docs.google.com/document/d/1Y_cSkXr-qQiFFlFoGqfzGHE9m9MnIvZSgGpFP5l5o4I/edit > > > >> > > >> > > > > >> > > >> > Let me know what you think. > > > >> > > >> > > > > >> > > >> > Thanks. > > > >> > > >> > Luke > > > >> > > >> > > > > >> > > >> > > > > >> > > >> > > > > >> > > >> > On Thu, Jul 25, 2024 at 8:07 PM Christo Lolov < > > > >> > christolo...@gmail.com > > > >> > > > > > > >> > > >> > wrote: > > > >> > > >> > > > > >> > > >> > > Hello! > > > >> > > >> > > > > > >> > > >> > > Thank you for raising this! > > > >> > > >> > > > > > >> > > >> > > Up to now KIP-950 took the stance that you can disable > > tiering > > > >> > > >> whenever > > > >> > > >> > you > > > >> > > >> > > wish as long as you specify what you would like to do > with > > the > > > >> > data > > > >> > > in > > > >> > > >> > > remote. Amongst other things it also made the promise > that > > it > > > >> will > > > >> > > not > > > >> > > >> > > delete data without a user explicitly saying that they > want > > > >> their > > > >> > > data > > > >> > > >> > > deleted. In other words there is a 2-step verification > > that the > > > >> > user > > > >> > > >> > truly > > > >> > > >> > > wants their data deleted. > > > >> > > >> > > > > > >> > > >> > > From the table of the new proposal I am left with the > > > >> impression > > > >> > > that > > > >> > > >> the > > > >> > > >> > > moment a user tries to disable tiering their data will by > > > >> deleted. > > > >> > > In > > > >> > > >> > other > > > >> > > >> > > words, there is no 2-step verification that they want > their > > > >> data > > > >> > > >> deleted. > > > >> > > >> > > > > > >> > > >> > > On a first read, I wouldn't be opposed to this proposal > > since > > > >> it > > > >> > > >> > provides a > > > >> > > >> > > neat alternative to the tiered epoch as long as there is > > still > > > >> a > > > >> > > >> 2-step > > > >> > > >> > > verification that the user is aware their data will be > > > >> deleted. I > > > >> > > >> think > > > >> > > >> > > that a reasonable way to achieve this is to keep the > > > >> > > >> > > remote.log.disable.policy, but only allow it to take a > > value of > > > >> > > >> "delete". > > > >> > > >> > > > > > >> > > >> > > What are your thoughts? > > > >> > > >> > > > > > >> > > >> > > Best, > > > >> > > >> > > Christo > > > >> > > >> > > > > > >> > > >> > > > > > >> > > >> > > On Thu, 25 Jul 2024 at 12:10, Luke Chen < > show...@gmail.com > > > > > > >> > wrote: > > > >> > > >> > > > > > >> > > >> > > > Hi all, > > > >> > > >> > > > > > > >> > > >> > > > I just found the table is not able to be displayed > > correctly > > > >> in > > > >> > > the > > > >> > > >> > > email. > > > >> > > >> > > > I've put the table content in google doc here > > > >> > > >> > > > < > > > >> > > >> > > > > > > >> > > >> > > > > > >> > > >> > > > > >> > > >> > > > >> > > > > > >> > > > > >> > > > https://docs.google.com/document/d/1Y_cSkXr-qQiFFlFoGqfzGHE9m9MnIvZSgGpFP5l5o4I/edit?usp=sharing > > > >> > > >> > > > > > > > >> > > >> > > > . > > > >> > > >> > > > > > > >> > > >> > > > Thanks. > > > >> > > >> > > > Luke > > > >> > > >> > > > > > > >> > > >> > > > On Thu, Jul 25, 2024 at 6:30 PM Luke Chen < > > show...@gmail.com > > > >> > > > > >> > > >> wrote: > > > >> > > >> > > > > > > >> > > >> > > > > Hi all, > > > >> > > >> > > > > > > > >> > > >> > > > > While implementing the feature in KRaft mode, I found > > > >> > something > > > >> > > we > > > >> > > >> > need > > > >> > > >> > > > to > > > >> > > >> > > > > change the original proposal: > > > >> > > >> > > > > > > > >> > > >> > > > > (1) In the KIP of "Disablement - KRaft backed Cluster > > > >> > > >> > > > > < > > > >> > > >> > > > > > > >> > > >> > > > > > >> > > >> > > > > >> > > >> > > > >> > > > > > >> > > > > >> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-950%3A++Tiered+Storage+Disablement#KIP950:TieredStorageDisablement-Disablement-KRaftBackedCluster > > > >> > > >> > > > >", > > > >> > > >> > > > > we said: > > > >> > > >> > > > > Controller persists configuration change and > completes > > > >> > > >> disablement: > > > >> > > >> > > > > > > > >> > > >> > > > > 1. The controller creates a ConfigRecord and > > persists > > > >> it in > > > >> > > the > > > >> > > >> > > > > metadata topic. > > > >> > > >> > > > > 2. The controller creates a TopicRecord to > > increment the > > > >> > > >> > > tiered_epoch > > > >> > > >> > > > > and update the tiered_state to DISABLED state. > > > >> > > >> > > > > 3. This update marks the completion of the > > disablement > > > >> > > process, > > > >> > > >> > > > > indicating that tiered storage has been > successfully > > > >> > disabled > > > >> > > >> for > > > >> > > >> > > the > > > >> > > >> > > > > KRaft-backed clusters. Similar to topic deletion > all > > > >> > replicas > > > >> > > >> will > > > >> > > >> > > > > eventually pick up the changes from the cluster > > metadata > > > >> > > topic > > > >> > > >> and > > > >> > > >> > > > apply > > > >> > > >> > > > > them to their own state. Any deletion failures > will > > be > > > >> > picked > > > >> > > >> up > > > >> > > >> > by > > > >> > > >> > > > the > > > >> > > >> > > > > expiration threads which should be deleting data > > before > > > >> the > > > >> > > log > > > >> > > >> > > start > > > >> > > >> > > > > offset. If the retention policy is delete, a new > > > >> expiration > > > >> > > >> thread > > > >> > > >> > > > will be > > > >> > > >> > > > > started on leadership change on any historical > > tiered > > > >> topic > > > >> > > to > > > >> > > >> > > > confirm that > > > >> > > >> > > > > there aren't any leftover segments in remote which > > need > > > >> > > >> deletion. > > > >> > > >> > > > After a > > > >> > > >> > > > > cycle in which it didn't delete anything, it will > > die. > > > >> > > >> > > > > > > > >> > > >> > > > > For the (b) step, I don't think the controller needs > to > > > >> create > > > >> > > >> > > > TopicRecord > > > >> > > >> > > > > because: > > > >> > > >> > > > > 1. The broker can fetch the "tiered_state" from the > > > >> > ConfigRecord > > > >> > > >> > > > > 2. The "tiered_epoch" is not necessary because raft > > > >> protocol > > > >> > > will > > > >> > > >> > keep > > > >> > > >> > > > the > > > >> > > >> > > > > order for us. The broker can rely on the raft > protocol > > and > > > >> > apply > > > >> > > >> them > > > >> > > >> > > in > > > >> > > >> > > > > order, to get the expected results. > > > >> > > >> > > > > 3. Marking the completion of the disablement process. > > In > > > >> > KRaft, > > > >> > > >> it's > > > >> > > >> > > not > > > >> > > >> > > > > necessary because once the ConfigRecord is accepted > by > > the > > > >> > > >> > controller, > > > >> > > >> > > it > > > >> > > >> > > > > must be applied by all the observers "in order". > > > >> > > >> > > > > > > > >> > > >> > > > > So, I'd like to propose to remove the (b) step in > KRaft > > > >> mode. > > > >> > > >> > > > > > > > >> > > >> > > > > (2) Current configuration make users and > implementation > > > >> > > confusion. > > > >> > > >> > > > > This is what originally we proposed in KIP-950: > > > >> > > >> > > > > > > > >> > > >> > > > > remote.storage.enable > > > >> > > >> > > > > > > > >> > > >> > > > > remote.log.disable.policy(new) > > > >> > > >> > > > > > > > >> > > >> > > > > remote storage data > > > >> > > >> > > > > > > > >> > > >> > > > > true > > > >> > > >> > > > > > > > >> > > >> > > > > null/retain/delete > > > >> > > >> > > > > > > > >> > > >> > > > > uploadable + readable > > > >> > > >> > > > > > > > >> > > >> > > > > false > > > >> > > >> > > > > > > > >> > > >> > > > > retain (default) > > > >> > > >> > > > > > > > >> > > >> > > > > readable, but remote storage is disabled? For users, > > they > > > >> are > > > >> > > also > > > >> > > >> > > > > surprised if this topic is reading data from remote > > > >> storage. > > > >> > > >> > > > > > > > >> > > >> > > > > Note: This also makes development difficult because > > it’s > > > >> > unable > > > >> > > to > > > >> > > >> > > > > distinguish between: > > > >> > > >> > > > > > > > >> > > >> > > > > (1) a topic never enables remote storage > > > >> > > >> > > > > > > > >> > > >> > > > > (2) a topic enabled and then disabled remote storage > > > >> > > >> > > > > > > > >> > > >> > > > > A problem we have is when broker startup and trying > to > > set > > > >> log > > > >> > > >> start > > > >> > > >> > > > > offset. Since the remote storage is disabled, we > > originally > > > >> > > should > > > >> > > >> > set > > > >> > > >> > > to > > > >> > > >> > > > > “local log start offset”, but in case (2), we expect > > it to > > > >> > treat > > > >> > > >> it > > > >> > > >> > as > > > >> > > >> > > > > “remote storage enabled”, which is confusing. > > > >> > > >> > > > > > > > >> > > >> > > > > false > > > >> > > >> > > > > > > > >> > > >> > > > > delete > > > >> > > >> > > > > > > > >> > > >> > > > > All remote data are deleted > > > >> > > >> > > > > > > > >> > > >> > > > > > > > >> > > >> > > > > Therefore, Kamal and I would like to propose a new > > version > > > >> of > > > >> > > the > > > >> > > >> > > > > configuration: > > > >> > > >> > > > > > > > >> > > >> > > > > remote.storage.enable > > > >> > > >> > > > > > > > >> > > >> > > > > remote.copy.disabled (new) > > > >> > > >> > > > > > > > >> > > >> > > > > remote storage data > > > >> > > >> > > > > > > > >> > > >> > > > > true > > > >> > > >> > > > > > > > >> > > >> > > > > false (default) > > > >> > > >> > > > > > > > >> > > >> > > > > uploadable + readable > > > >> > > >> > > > > > > > >> > > >> > > > > true > > > >> > > >> > > > > > > > >> > > >> > > > > true > > > >> > > >> > > > > > > > >> > > >> > > > > readable > > > >> > > >> > > > > > > > >> > > >> > > > > false > > > >> > > >> > > > > > > > >> > > >> > > > > true/false > > > >> > > >> > > > > > > > >> > > >> > > > > All remote data are deleted > > > >> > > >> > > > > > > > >> > > >> > > > > The advantage is this config makes users clear what > it > > is > > > >> > > >> > configuring, > > > >> > > >> > > > and > > > >> > > >> > > > > the result is expected. > > > >> > > >> > > > > Also, on the implementation side, we can still rely > on > > > >> > > >> > > > > "remote.storage.enable" to identify is this feature > is > > > >> on/off. > > > >> > > >> > > > > > > > >> > > >> > > > > Any thoughts about it? > > > >> > > >> > > > > > > > >> > > >> > > > > Thank you. > > > >> > > >> > > > > Luke > > > >> > > >> > > > > > > > >> > > >> > > > > > > > >> > > >> > > > > > > > >> > > >> > > > > On Thu, May 30, 2024 at 6:50 PM David Jacot > > > >> > > >> > > <dja...@confluent.io.invalid > > > >> > > >> > > > > > > > >> > > >> > > > > wrote: > > > >> > > >> > > > > > > > >> > > >> > > > >> Hi all, > > > >> > > >> > > > >> > > > >> > > >> > > > >> Thanks for the KIP. This is definitely a worthwhile > > > >> feature. > > > >> > > >> > However, > > > >> > > >> > > I > > > >> > > >> > > > am > > > >> > > >> > > > >> a bit sceptical on the ZK part of the story. The 3.8 > > > >> release > > > >> > is > > > >> > > >> > > supposed > > > >> > > >> > > > >> to > > > >> > > >> > > > >> be the last one supporting ZK so I don't really see > > how we > > > >> > > could > > > >> > > >> > bring > > > >> > > >> > > > it > > > >> > > >> > > > >> to ZK, knowing that we don't plan to do a 3.9 > release > > > >> > (current > > > >> > > >> > plan). > > > >> > > >> > > I > > > >> > > >> > > > >> strongly suggest clarifying this before implementing > > the > > > >> ZK > > > >> > > part > > > >> > > >> in > > > >> > > >> > > > order > > > >> > > >> > > > >> to avoid having new code [1] being deleted right > after > > > >> 3.8 is > > > >> > > >> > released > > > >> > > >> > > > >> :). Personally, I agree with Chia-Ping and Mickael. > We > > > >> could > > > >> > > drop > > > >> > > >> > the > > > >> > > >> > > ZK > > > >> > > >> > > > >> part. > > > >> > > >> > > > >> > > > >> > > >> > > > >> [1] https://github.com/apache/kafka/pull/16131 > > > >> > > >> > > > >> > > > >> > > >> > > > >> Best, > > > >> > > >> > > > >> David > > > >> > > >> > > > >> > > > >> > > >> > > > >> On Tue, May 28, 2024 at 1:31 PM Mickael Maison < > > > >> > > >> > > > mickael.mai...@gmail.com> > > > >> > > >> > > > >> wrote: > > > >> > > >> > > > >> > > > >> > > >> > > > >> > Hi, > > > >> > > >> > > > >> > > > > >> > > >> > > > >> > I agree with Chia-Ping, I think we could drop the > ZK > > > >> > variant > > > >> > > >> > > > >> > altogether, especially if this is not going to > make > > it > > > >> in > > > >> > > >> 3.8.0. > > > >> > > >> > > > >> > Even if we end up needing a 3.9.0 release, I > > wouldn't > > > >> > write a > > > >> > > >> > bunch > > > >> > > >> > > of > > > >> > > >> > > > >> > new ZooKeeper-related code in that release to > > delete it > > > >> all > > > >> > > >> right > > > >> > > >> > > > >> > after in 4.0. > > > >> > > >> > > > >> > > > > >> > > >> > > > >> > Thanks, > > > >> > > >> > > > >> > Mickael > > > >> > > >> > > > >> > > > > >> > > >> > > > >> > On Fri, May 24, 2024 at 5:03 PM Christo Lolov < > > > >> > > >> > > christolo...@gmail.com > > > >> > > >> > > > > > > > >> > > >> > > > >> > wrote: > > > >> > > >> > > > >> > > > > > >> > > >> > > > >> > > Hello! > > > >> > > >> > > > >> > > > > > >> > > >> > > > >> > > I am closing this vote as ACCEPTED with 3 > binding > > +1 > > > >> > (Luke, > > > >> > > >> > > > Chia-Ping > > > >> > > >> > > > >> and > > > >> > > >> > > > >> > > Satish) and 1 non-binding +1 (Kamal) - thank you > > for > > > >> the > > > >> > > >> > reviews! > > > >> > > >> > > > >> > > > > > >> > > >> > > > >> > > Realistically, I don't think I have the > bandwidth > > to > > > >> get > > > >> > > >> this in > > > >> > > >> > > > >> 3.8.0. > > > >> > > >> > > > >> > > Due to this, I will mark tentatively the > Zookeeper > > > >> part > > > >> > for > > > >> > > >> 3.9 > > > >> > > >> > if > > > >> > > >> > > > the > > > >> > > >> > > > >> > > community decides that they do in fact want one > > more > > > >> 3.x > > > >> > > >> > release. > > > >> > > >> > > > >> > > I will mark the KRaft part as ready to be > started > > and > > > >> > > aiming > > > >> > > >> for > > > >> > > >> > > > >> either > > > >> > > >> > > > >> > 4.0 > > > >> > > >> > > > >> > > or 3.9. > > > >> > > >> > > > >> > > > > > >> > > >> > > > >> > > Best, > > > >> > > >> > > > >> > > Christo > > > >> > > >> > > > >> > > > > >> > > >> > > > >> > > > >> > > >> > > > > > > > >> > > >> > > > > > > >> > > >> > > > > > >> > > >> > > > > >> > > >> > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > > > >