Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi Jun, Ok sounds good. Justine On Fri, Apr 12, 2024 at 10:17 AM Jun Rao wrote: > Hi, Justine, > > unstable.metadata.versions.enable is an internal configuration. So, we > could probably just remove it instead of depreciation. Also, it would be > useful to make it clear that unstable.feature.versions.enable is an > internal configuration. > > Thanks, > > Jun > > On Thu, Apr 11, 2024 at 11:16 AM Justine Olshan > wrote: > > > Updated. :) > > Thanks for the reviews > > > > Justine > > > > On Thu, Apr 11, 2024 at 11:01 AM Jun Rao > wrote: > > > > > Hi, Justine, > > > > > > Thanks for the updated KIP. > > > > > > Perhaps it's better to name the new config > > unstable.feature.versions.enable > > > since there could be multiple unstable versions. > > > > > > Other than that, the KIP looks good to me. > > > > > > Jun > > > > > > On Thu, Apr 11, 2024 at 9:06 AM Justine Olshan > > > > > > wrote: > > > > > > > The original config was never actually approved in any KIP. But we > can > > > say > > > > it is deprecated. > > > > I can change the config name. > > > > > > > > Justine > > > > > > > > On Thu, Apr 11, 2024 at 8:52 AM Jun Rao > > > wrote: > > > > > > > > > Hi, Justine, > > > > > > > > > > Thanks for the updated KIP. > > > > > > > > > > Would unstable.feature.version.enable be a clearer name? Also, > should > > > we > > > > > remove/deprecate unstable.metadata.versions.enable in this KIP? > > > > > > > > > > Jun > > > > > > > > > > On Tue, Apr 9, 2024 at 9:09 AM Justine Olshan > > > > > > > > > > > > > > wrote: > > > > > > > > > > > Hi Jun, > > > > > > > > > > > > Makes sense to me. It seems like KIP-1014 has been inactive > > > recently. I > > > > > can > > > > > > update my KIP and mention this change on that discussion thread. > > > > > > > > > > > > Justine > > > > > > > > > > > > On Tue, Apr 9, 2024 at 9:01 AM Jun Rao > > > > > wrote: > > > > > > > > > > > > > Hi, Justine, > > > > > > > > > > > > > > A single config makes sense to me too. We just need to reach > > > > consensus > > > > > > with > > > > > > > KIP-1014. > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > Jun > > > > > > > > > > > > > > On Mon, Apr 8, 2024 at 5:06 PM Justine Olshan > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > Hey Jun, > > > > > > > > > > > > > > > > That's a good question. I think maybe for simplicity, we can > > > have a > > > > > > > single > > > > > > > > config? > > > > > > > > If that makes sense, I will update the KIP. > > > > > > > > > > > > > > > > Justine > > > > > > > > > > > > > > > > On Mon, Apr 8, 2024 at 3:20 PM Jun Rao > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > Hi, Justine, > > > > > > > > > > > > > > > > > > Thanks for the updated KIP. > > > > > > > > > > > > > > > > > > One more question related to KIP-1014. It introduced a new > > > > > > > > > config unstable.metadata.versions.enable. Does each new > > feature > > > > > need > > > > > > to > > > > > > > > > have a corresponding config to enable the testing of > unstable > > > > > > features > > > > > > > or > > > > > > > > > should we have a generic config enabling the testing of all > > > > > unstable > > > > > > > > > features? > > > > > > > > > > > > > > > > > > Jun > > > > > > > > > > > > > > > > > > On Thu, Apr 4, 2024 at 8:24 PM Justine Olshan > > > > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > I'm hoping this covers the majority of comments. I will > go > > > > ahead > > > > > > and > > > > > > > > open > > > > > > > > > > the vote in the next day or so. > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > Justine > > > > > > > > > > > > > > > > > > > > On Wed, Apr 3, 2024 at 3:31 PM Justine Olshan < > > > > > > jols...@confluent.io> > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > Find and replace has failed me :( > > > > > > > > > > > > > > > > > > > > > > Group version seems a little vague, but we can update > it. > > > > > > Hopefully > > > > > > > > > find > > > > > > > > > > > and replace won't fail me again, otherwise I will get > > > another > > > > > > email > > > > > > > > on > > > > > > > > > > this. > > > > > > > > > > > > > > > > > > > > > > Justine > > > > > > > > > > > > > > > > > > > > > > On Wed, Apr 3, 2024 at 12:15 PM David Jacot > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > >> Thanks, Justine. > > > > > > > > > > >> > > > > > > > > > > >> * Should we also use `group.version` (GV) as I > suggested > > > in > > > > my > > > > > > > > > previous > > > > > > > > > > >> message in order to be consistent? > > > > > > > > > > >> * Should we add both names to the `Public Interfaces` > > > > section? > > > > > > > > > > >> * There is still at least one usage of > > > > > > > > `transaction.protocol.verison` > > > > > > > > > in > > > > > > > > > > >> the KIP too. > > > > > > > > > > >> > > > > > > > > > > >> Best, > > >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi, Justine, unstable.metadata.versions.enable is an internal configuration. So, we could probably just remove it instead of depreciation. Also, it would be useful to make it clear that unstable.feature.versions.enable is an internal configuration. Thanks, Jun On Thu, Apr 11, 2024 at 11:16 AM Justine Olshan wrote: > Updated. :) > Thanks for the reviews > > Justine > > On Thu, Apr 11, 2024 at 11:01 AM Jun Rao wrote: > > > Hi, Justine, > > > > Thanks for the updated KIP. > > > > Perhaps it's better to name the new config > unstable.feature.versions.enable > > since there could be multiple unstable versions. > > > > Other than that, the KIP looks good to me. > > > > Jun > > > > On Thu, Apr 11, 2024 at 9:06 AM Justine Olshan > > > > wrote: > > > > > The original config was never actually approved in any KIP. But we can > > say > > > it is deprecated. > > > I can change the config name. > > > > > > Justine > > > > > > On Thu, Apr 11, 2024 at 8:52 AM Jun Rao > > wrote: > > > > > > > Hi, Justine, > > > > > > > > Thanks for the updated KIP. > > > > > > > > Would unstable.feature.version.enable be a clearer name? Also, should > > we > > > > remove/deprecate unstable.metadata.versions.enable in this KIP? > > > > > > > > Jun > > > > > > > > On Tue, Apr 9, 2024 at 9:09 AM Justine Olshan > > > > > > > > > > > wrote: > > > > > > > > > Hi Jun, > > > > > > > > > > Makes sense to me. It seems like KIP-1014 has been inactive > > recently. I > > > > can > > > > > update my KIP and mention this change on that discussion thread. > > > > > > > > > > Justine > > > > > > > > > > On Tue, Apr 9, 2024 at 9:01 AM Jun Rao > > > wrote: > > > > > > > > > > > Hi, Justine, > > > > > > > > > > > > A single config makes sense to me too. We just need to reach > > > consensus > > > > > with > > > > > > KIP-1014. > > > > > > > > > > > > Thanks, > > > > > > > > > > > > Jun > > > > > > > > > > > > On Mon, Apr 8, 2024 at 5:06 PM Justine Olshan > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > Hey Jun, > > > > > > > > > > > > > > That's a good question. I think maybe for simplicity, we can > > have a > > > > > > single > > > > > > > config? > > > > > > > If that makes sense, I will update the KIP. > > > > > > > > > > > > > > Justine > > > > > > > > > > > > > > On Mon, Apr 8, 2024 at 3:20 PM Jun Rao > > > > > > > > wrote: > > > > > > > > > > > > > > > Hi, Justine, > > > > > > > > > > > > > > > > Thanks for the updated KIP. > > > > > > > > > > > > > > > > One more question related to KIP-1014. It introduced a new > > > > > > > > config unstable.metadata.versions.enable. Does each new > feature > > > > need > > > > > to > > > > > > > > have a corresponding config to enable the testing of unstable > > > > > features > > > > > > or > > > > > > > > should we have a generic config enabling the testing of all > > > > unstable > > > > > > > > features? > > > > > > > > > > > > > > > > Jun > > > > > > > > > > > > > > > > On Thu, Apr 4, 2024 at 8:24 PM Justine Olshan > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > I'm hoping this covers the majority of comments. I will go > > > ahead > > > > > and > > > > > > > open > > > > > > > > > the vote in the next day or so. > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > Justine > > > > > > > > > > > > > > > > > > On Wed, Apr 3, 2024 at 3:31 PM Justine Olshan < > > > > > jols...@confluent.io> > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > Find and replace has failed me :( > > > > > > > > > > > > > > > > > > > > Group version seems a little vague, but we can update it. > > > > > Hopefully > > > > > > > > find > > > > > > > > > > and replace won't fail me again, otherwise I will get > > another > > > > > email > > > > > > > on > > > > > > > > > this. > > > > > > > > > > > > > > > > > > > > Justine > > > > > > > > > > > > > > > > > > > > On Wed, Apr 3, 2024 at 12:15 PM David Jacot > > > > > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > >> Thanks, Justine. > > > > > > > > > >> > > > > > > > > > >> * Should we also use `group.version` (GV) as I suggested > > in > > > my > > > > > > > > previous > > > > > > > > > >> message in order to be consistent? > > > > > > > > > >> * Should we add both names to the `Public Interfaces` > > > section? > > > > > > > > > >> * There is still at least one usage of > > > > > > > `transaction.protocol.verison` > > > > > > > > in > > > > > > > > > >> the KIP too. > > > > > > > > > >> > > > > > > > > > >> Best, > > > > > > > > > >> David > > > > > > > > > >> > > > > > > > > > >> On Wed, Apr 3, 2024 at 6:29 PM Justine Olshan > > > > > > > > > >> > > > > > > > > > >> wrote: > > > > > > > > > >> > > > > > > > > > >> > I had missed the David's message yesterday about the > > > naming > > > > > for > > > > > > > > > >> transaction > > > > > > > > > >> > version vs transaction protocol version. > > > > > > > > > >> > > > > > > > > > > >> > After some
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Updated. :) Thanks for the reviews Justine On Thu, Apr 11, 2024 at 11:01 AM Jun Rao wrote: > Hi, Justine, > > Thanks for the updated KIP. > > Perhaps it's better to name the new config unstable.feature.versions.enable > since there could be multiple unstable versions. > > Other than that, the KIP looks good to me. > > Jun > > On Thu, Apr 11, 2024 at 9:06 AM Justine Olshan > > wrote: > > > The original config was never actually approved in any KIP. But we can > say > > it is deprecated. > > I can change the config name. > > > > Justine > > > > On Thu, Apr 11, 2024 at 8:52 AM Jun Rao > wrote: > > > > > Hi, Justine, > > > > > > Thanks for the updated KIP. > > > > > > Would unstable.feature.version.enable be a clearer name? Also, should > we > > > remove/deprecate unstable.metadata.versions.enable in this KIP? > > > > > > Jun > > > > > > On Tue, Apr 9, 2024 at 9:09 AM Justine Olshan > > > > > > > > wrote: > > > > > > > Hi Jun, > > > > > > > > Makes sense to me. It seems like KIP-1014 has been inactive > recently. I > > > can > > > > update my KIP and mention this change on that discussion thread. > > > > > > > > Justine > > > > > > > > On Tue, Apr 9, 2024 at 9:01 AM Jun Rao > > wrote: > > > > > > > > > Hi, Justine, > > > > > > > > > > A single config makes sense to me too. We just need to reach > > consensus > > > > with > > > > > KIP-1014. > > > > > > > > > > Thanks, > > > > > > > > > > Jun > > > > > > > > > > On Mon, Apr 8, 2024 at 5:06 PM Justine Olshan > > > > > > > > > > > > > > wrote: > > > > > > > > > > > Hey Jun, > > > > > > > > > > > > That's a good question. I think maybe for simplicity, we can > have a > > > > > single > > > > > > config? > > > > > > If that makes sense, I will update the KIP. > > > > > > > > > > > > Justine > > > > > > > > > > > > On Mon, Apr 8, 2024 at 3:20 PM Jun Rao > > > > > wrote: > > > > > > > > > > > > > Hi, Justine, > > > > > > > > > > > > > > Thanks for the updated KIP. > > > > > > > > > > > > > > One more question related to KIP-1014. It introduced a new > > > > > > > config unstable.metadata.versions.enable. Does each new feature > > > need > > > > to > > > > > > > have a corresponding config to enable the testing of unstable > > > > features > > > > > or > > > > > > > should we have a generic config enabling the testing of all > > > unstable > > > > > > > features? > > > > > > > > > > > > > > Jun > > > > > > > > > > > > > > On Thu, Apr 4, 2024 at 8:24 PM Justine Olshan > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > I'm hoping this covers the majority of comments. I will go > > ahead > > > > and > > > > > > open > > > > > > > > the vote in the next day or so. > > > > > > > > > > > > > > > > Thanks, > > > > > > > > Justine > > > > > > > > > > > > > > > > On Wed, Apr 3, 2024 at 3:31 PM Justine Olshan < > > > > jols...@confluent.io> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > Find and replace has failed me :( > > > > > > > > > > > > > > > > > > Group version seems a little vague, but we can update it. > > > > Hopefully > > > > > > > find > > > > > > > > > and replace won't fail me again, otherwise I will get > another > > > > email > > > > > > on > > > > > > > > this. > > > > > > > > > > > > > > > > > > Justine > > > > > > > > > > > > > > > > > > On Wed, Apr 3, 2024 at 12:15 PM David Jacot > > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > >> Thanks, Justine. > > > > > > > > >> > > > > > > > > >> * Should we also use `group.version` (GV) as I suggested > in > > my > > > > > > > previous > > > > > > > > >> message in order to be consistent? > > > > > > > > >> * Should we add both names to the `Public Interfaces` > > section? > > > > > > > > >> * There is still at least one usage of > > > > > > `transaction.protocol.verison` > > > > > > > in > > > > > > > > >> the KIP too. > > > > > > > > >> > > > > > > > > >> Best, > > > > > > > > >> David > > > > > > > > >> > > > > > > > > >> On Wed, Apr 3, 2024 at 6:29 PM Justine Olshan > > > > > > > > >> > > > > > > > > >> wrote: > > > > > > > > >> > > > > > > > > >> > I had missed the David's message yesterday about the > > naming > > > > for > > > > > > > > >> transaction > > > > > > > > >> > version vs transaction protocol version. > > > > > > > > >> > > > > > > > > > >> > After some offline discussion with Jun, Artem, and > David, > > we > > > > > > agreed > > > > > > > > that > > > > > > > > >> > transaction version is simpler and conveys more than > just > > > > > protocol > > > > > > > > >> changes > > > > > > > > >> > (flexible records for example) > > > > > > > > >> > > > > > > > > > >> > I will update the KIP as well as KIP-890 > > > > > > > > >> > > > > > > > > > >> > Thanks, > > > > > > > > >> > Justine > > > > > > > > >> > > > > > > > > > >> > On Tue, Apr 2, 2024 at 2:50 PM Justine Olshan < > > > > > > jols...@confluent.io > > > > > > > > > > > > > > > > >> > wrote: > > > > > > > > >> > > > > > > > > > >> > > Updated! > > > > > > >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi, Justine, Thanks for the updated KIP. Perhaps it's better to name the new config unstable.feature.versions.enable since there could be multiple unstable versions. Other than that, the KIP looks good to me. Jun On Thu, Apr 11, 2024 at 9:06 AM Justine Olshan wrote: > The original config was never actually approved in any KIP. But we can say > it is deprecated. > I can change the config name. > > Justine > > On Thu, Apr 11, 2024 at 8:52 AM Jun Rao wrote: > > > Hi, Justine, > > > > Thanks for the updated KIP. > > > > Would unstable.feature.version.enable be a clearer name? Also, should we > > remove/deprecate unstable.metadata.versions.enable in this KIP? > > > > Jun > > > > On Tue, Apr 9, 2024 at 9:09 AM Justine Olshan > > > > > wrote: > > > > > Hi Jun, > > > > > > Makes sense to me. It seems like KIP-1014 has been inactive recently. I > > can > > > update my KIP and mention this change on that discussion thread. > > > > > > Justine > > > > > > On Tue, Apr 9, 2024 at 9:01 AM Jun Rao > wrote: > > > > > > > Hi, Justine, > > > > > > > > A single config makes sense to me too. We just need to reach > consensus > > > with > > > > KIP-1014. > > > > > > > > Thanks, > > > > > > > > Jun > > > > > > > > On Mon, Apr 8, 2024 at 5:06 PM Justine Olshan > > > > > > > > > > > wrote: > > > > > > > > > Hey Jun, > > > > > > > > > > That's a good question. I think maybe for simplicity, we can have a > > > > single > > > > > config? > > > > > If that makes sense, I will update the KIP. > > > > > > > > > > Justine > > > > > > > > > > On Mon, Apr 8, 2024 at 3:20 PM Jun Rao > > > wrote: > > > > > > > > > > > Hi, Justine, > > > > > > > > > > > > Thanks for the updated KIP. > > > > > > > > > > > > One more question related to KIP-1014. It introduced a new > > > > > > config unstable.metadata.versions.enable. Does each new feature > > need > > > to > > > > > > have a corresponding config to enable the testing of unstable > > > features > > > > or > > > > > > should we have a generic config enabling the testing of all > > unstable > > > > > > features? > > > > > > > > > > > > Jun > > > > > > > > > > > > On Thu, Apr 4, 2024 at 8:24 PM Justine Olshan > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > I'm hoping this covers the majority of comments. I will go > ahead > > > and > > > > > open > > > > > > > the vote in the next day or so. > > > > > > > > > > > > > > Thanks, > > > > > > > Justine > > > > > > > > > > > > > > On Wed, Apr 3, 2024 at 3:31 PM Justine Olshan < > > > jols...@confluent.io> > > > > > > > wrote: > > > > > > > > > > > > > > > Find and replace has failed me :( > > > > > > > > > > > > > > > > Group version seems a little vague, but we can update it. > > > Hopefully > > > > > > find > > > > > > > > and replace won't fail me again, otherwise I will get another > > > email > > > > > on > > > > > > > this. > > > > > > > > > > > > > > > > Justine > > > > > > > > > > > > > > > > On Wed, Apr 3, 2024 at 12:15 PM David Jacot > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > >> Thanks, Justine. > > > > > > > >> > > > > > > > >> * Should we also use `group.version` (GV) as I suggested in > my > > > > > > previous > > > > > > > >> message in order to be consistent? > > > > > > > >> * Should we add both names to the `Public Interfaces` > section? > > > > > > > >> * There is still at least one usage of > > > > > `transaction.protocol.verison` > > > > > > in > > > > > > > >> the KIP too. > > > > > > > >> > > > > > > > >> Best, > > > > > > > >> David > > > > > > > >> > > > > > > > >> On Wed, Apr 3, 2024 at 6:29 PM Justine Olshan > > > > > > > >> > > > > > > > >> wrote: > > > > > > > >> > > > > > > > >> > I had missed the David's message yesterday about the > naming > > > for > > > > > > > >> transaction > > > > > > > >> > version vs transaction protocol version. > > > > > > > >> > > > > > > > > >> > After some offline discussion with Jun, Artem, and David, > we > > > > > agreed > > > > > > > that > > > > > > > >> > transaction version is simpler and conveys more than just > > > > protocol > > > > > > > >> changes > > > > > > > >> > (flexible records for example) > > > > > > > >> > > > > > > > > >> > I will update the KIP as well as KIP-890 > > > > > > > >> > > > > > > > > >> > Thanks, > > > > > > > >> > Justine > > > > > > > >> > > > > > > > > >> > On Tue, Apr 2, 2024 at 2:50 PM Justine Olshan < > > > > > jols...@confluent.io > > > > > > > > > > > > > > >> > wrote: > > > > > > > >> > > > > > > > > >> > > Updated! > > > > > > > >> > > > > > > > > > >> > > Justine > > > > > > > >> > > > > > > > > > >> > > On Tue, Apr 2, 2024 at 2:40 PM Jun Rao > > > > > > > > > > > > > > > > >> wrote: > > > > > > > >> > > > > > > > > > >> > >> Hi, Justine, > > > > > > > >> > >> > > > > > > > >> > >> Thanks for the reply. > > > > > > > >> > >> > > > > > > > >> > >> 21. Sounds good. It would be useful to document that. > > > > > > > >> > >> > > > > > > > >> > >> 22. Should we add the IV in
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
The original config was never actually approved in any KIP. But we can say it is deprecated. I can change the config name. Justine On Thu, Apr 11, 2024 at 8:52 AM Jun Rao wrote: > Hi, Justine, > > Thanks for the updated KIP. > > Would unstable.feature.version.enable be a clearer name? Also, should we > remove/deprecate unstable.metadata.versions.enable in this KIP? > > Jun > > On Tue, Apr 9, 2024 at 9:09 AM Justine Olshan > > wrote: > > > Hi Jun, > > > > Makes sense to me. It seems like KIP-1014 has been inactive recently. I > can > > update my KIP and mention this change on that discussion thread. > > > > Justine > > > > On Tue, Apr 9, 2024 at 9:01 AM Jun Rao wrote: > > > > > Hi, Justine, > > > > > > A single config makes sense to me too. We just need to reach consensus > > with > > > KIP-1014. > > > > > > Thanks, > > > > > > Jun > > > > > > On Mon, Apr 8, 2024 at 5:06 PM Justine Olshan > > > > > > > > wrote: > > > > > > > Hey Jun, > > > > > > > > That's a good question. I think maybe for simplicity, we can have a > > > single > > > > config? > > > > If that makes sense, I will update the KIP. > > > > > > > > Justine > > > > > > > > On Mon, Apr 8, 2024 at 3:20 PM Jun Rao > > wrote: > > > > > > > > > Hi, Justine, > > > > > > > > > > Thanks for the updated KIP. > > > > > > > > > > One more question related to KIP-1014. It introduced a new > > > > > config unstable.metadata.versions.enable. Does each new feature > need > > to > > > > > have a corresponding config to enable the testing of unstable > > features > > > or > > > > > should we have a generic config enabling the testing of all > unstable > > > > > features? > > > > > > > > > > Jun > > > > > > > > > > On Thu, Apr 4, 2024 at 8:24 PM Justine Olshan > > > > > > > > > > > > > > wrote: > > > > > > > > > > > I'm hoping this covers the majority of comments. I will go ahead > > and > > > > open > > > > > > the vote in the next day or so. > > > > > > > > > > > > Thanks, > > > > > > Justine > > > > > > > > > > > > On Wed, Apr 3, 2024 at 3:31 PM Justine Olshan < > > jols...@confluent.io> > > > > > > wrote: > > > > > > > > > > > > > Find and replace has failed me :( > > > > > > > > > > > > > > Group version seems a little vague, but we can update it. > > Hopefully > > > > > find > > > > > > > and replace won't fail me again, otherwise I will get another > > email > > > > on > > > > > > this. > > > > > > > > > > > > > > Justine > > > > > > > > > > > > > > On Wed, Apr 3, 2024 at 12:15 PM David Jacot > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > >> Thanks, Justine. > > > > > > >> > > > > > > >> * Should we also use `group.version` (GV) as I suggested in my > > > > > previous > > > > > > >> message in order to be consistent? > > > > > > >> * Should we add both names to the `Public Interfaces` section? > > > > > > >> * There is still at least one usage of > > > > `transaction.protocol.verison` > > > > > in > > > > > > >> the KIP too. > > > > > > >> > > > > > > >> Best, > > > > > > >> David > > > > > > >> > > > > > > >> On Wed, Apr 3, 2024 at 6:29 PM Justine Olshan > > > > > > >> > > > > > > >> wrote: > > > > > > >> > > > > > > >> > I had missed the David's message yesterday about the naming > > for > > > > > > >> transaction > > > > > > >> > version vs transaction protocol version. > > > > > > >> > > > > > > > >> > After some offline discussion with Jun, Artem, and David, we > > > > agreed > > > > > > that > > > > > > >> > transaction version is simpler and conveys more than just > > > protocol > > > > > > >> changes > > > > > > >> > (flexible records for example) > > > > > > >> > > > > > > > >> > I will update the KIP as well as KIP-890 > > > > > > >> > > > > > > > >> > Thanks, > > > > > > >> > Justine > > > > > > >> > > > > > > > >> > On Tue, Apr 2, 2024 at 2:50 PM Justine Olshan < > > > > jols...@confluent.io > > > > > > > > > > > > >> > wrote: > > > > > > >> > > > > > > > >> > > Updated! > > > > > > >> > > > > > > > > >> > > Justine > > > > > > >> > > > > > > > > >> > > On Tue, Apr 2, 2024 at 2:40 PM Jun Rao > > > > > > > > > > > > > >> wrote: > > > > > > >> > > > > > > > > >> > >> Hi, Justine, > > > > > > >> > >> > > > > > > >> > >> Thanks for the reply. > > > > > > >> > >> > > > > > > >> > >> 21. Sounds good. It would be useful to document that. > > > > > > >> > >> > > > > > > >> > >> 22. Should we add the IV in "metadata.version=17 has no > > > > > > dependencies" > > > > > > >> > too? > > > > > > >> > >> > > > > > > >> > >> Jun > > > > > > >> > >> > > > > > > >> > >> > > > > > > >> > >> On Tue, Apr 2, 2024 at 11:31 AM Justine Olshan > > > > > > >> > >> > > > > > > >> > >> wrote: > > > > > > >> > >> > > > > > > >> > >> > Jun, > > > > > > >> > >> > > > > > > > >> > >> > 21. Next producer ID field doesn't need to be populated > > for > > > > TV > > > > > 1. > > > > > > >> We > > > > > > >> > >> don't > > > > > > >> > >> > have the same need to retain this since it is written > > > > directly > > > > > to > > > > > > >> the >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi, Justine, Thanks for the updated KIP. Would unstable.feature.version.enable be a clearer name? Also, should we remove/deprecate unstable.metadata.versions.enable in this KIP? Jun On Tue, Apr 9, 2024 at 9:09 AM Justine Olshan wrote: > Hi Jun, > > Makes sense to me. It seems like KIP-1014 has been inactive recently. I can > update my KIP and mention this change on that discussion thread. > > Justine > > On Tue, Apr 9, 2024 at 9:01 AM Jun Rao wrote: > > > Hi, Justine, > > > > A single config makes sense to me too. We just need to reach consensus > with > > KIP-1014. > > > > Thanks, > > > > Jun > > > > On Mon, Apr 8, 2024 at 5:06 PM Justine Olshan > > > > > wrote: > > > > > Hey Jun, > > > > > > That's a good question. I think maybe for simplicity, we can have a > > single > > > config? > > > If that makes sense, I will update the KIP. > > > > > > Justine > > > > > > On Mon, Apr 8, 2024 at 3:20 PM Jun Rao > wrote: > > > > > > > Hi, Justine, > > > > > > > > Thanks for the updated KIP. > > > > > > > > One more question related to KIP-1014. It introduced a new > > > > config unstable.metadata.versions.enable. Does each new feature need > to > > > > have a corresponding config to enable the testing of unstable > features > > or > > > > should we have a generic config enabling the testing of all unstable > > > > features? > > > > > > > > Jun > > > > > > > > On Thu, Apr 4, 2024 at 8:24 PM Justine Olshan > > > > > > > > > > > wrote: > > > > > > > > > I'm hoping this covers the majority of comments. I will go ahead > and > > > open > > > > > the vote in the next day or so. > > > > > > > > > > Thanks, > > > > > Justine > > > > > > > > > > On Wed, Apr 3, 2024 at 3:31 PM Justine Olshan < > jols...@confluent.io> > > > > > wrote: > > > > > > > > > > > Find and replace has failed me :( > > > > > > > > > > > > Group version seems a little vague, but we can update it. > Hopefully > > > > find > > > > > > and replace won't fail me again, otherwise I will get another > email > > > on > > > > > this. > > > > > > > > > > > > Justine > > > > > > > > > > > > On Wed, Apr 3, 2024 at 12:15 PM David Jacot > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > >> Thanks, Justine. > > > > > >> > > > > > >> * Should we also use `group.version` (GV) as I suggested in my > > > > previous > > > > > >> message in order to be consistent? > > > > > >> * Should we add both names to the `Public Interfaces` section? > > > > > >> * There is still at least one usage of > > > `transaction.protocol.verison` > > > > in > > > > > >> the KIP too. > > > > > >> > > > > > >> Best, > > > > > >> David > > > > > >> > > > > > >> On Wed, Apr 3, 2024 at 6:29 PM Justine Olshan > > > > > >> > > > > > >> wrote: > > > > > >> > > > > > >> > I had missed the David's message yesterday about the naming > for > > > > > >> transaction > > > > > >> > version vs transaction protocol version. > > > > > >> > > > > > > >> > After some offline discussion with Jun, Artem, and David, we > > > agreed > > > > > that > > > > > >> > transaction version is simpler and conveys more than just > > protocol > > > > > >> changes > > > > > >> > (flexible records for example) > > > > > >> > > > > > > >> > I will update the KIP as well as KIP-890 > > > > > >> > > > > > > >> > Thanks, > > > > > >> > Justine > > > > > >> > > > > > > >> > On Tue, Apr 2, 2024 at 2:50 PM Justine Olshan < > > > jols...@confluent.io > > > > > > > > > > >> > wrote: > > > > > >> > > > > > > >> > > Updated! > > > > > >> > > > > > > > >> > > Justine > > > > > >> > > > > > > > >> > > On Tue, Apr 2, 2024 at 2:40 PM Jun Rao > > > > > > > > > > >> wrote: > > > > > >> > > > > > > > >> > >> Hi, Justine, > > > > > >> > >> > > > > > >> > >> Thanks for the reply. > > > > > >> > >> > > > > > >> > >> 21. Sounds good. It would be useful to document that. > > > > > >> > >> > > > > > >> > >> 22. Should we add the IV in "metadata.version=17 has no > > > > > dependencies" > > > > > >> > too? > > > > > >> > >> > > > > > >> > >> Jun > > > > > >> > >> > > > > > >> > >> > > > > > >> > >> On Tue, Apr 2, 2024 at 11:31 AM Justine Olshan > > > > > >> > >> > > > > > >> > >> wrote: > > > > > >> > >> > > > > > >> > >> > Jun, > > > > > >> > >> > > > > > > >> > >> > 21. Next producer ID field doesn't need to be populated > for > > > TV > > > > 1. > > > > > >> We > > > > > >> > >> don't > > > > > >> > >> > have the same need to retain this since it is written > > > directly > > > > to > > > > > >> the > > > > > >> > >> > transaction log in InitProducerId. It is only needed for > > > > KIP-890 > > > > > >> part > > > > > >> > 2 > > > > > >> > >> / > > > > > >> > >> > TV 2. > > > > > >> > >> > > > > > > >> > >> > 22. We can do that. > > > > > >> > >> > > > > > > >> > >> > Justine > > > > > >> > >> > > > > > > >> > >> > On Tue, Apr 2, 2024 at 10:41 AM Jun Rao > > > > > > > > > > > > > > >> > >> wrote: > > > > > >> > >> > > > > > > >> > >> > > Hi, Justine, > > > > > >> > >> > > > > > > > >> > >> > > Thanks for the reply. > > > > >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi Justine, On Tue, Apr 9, 2024 at 4:19 PM Justine Olshan wrote: > As for the validation criteria. It seems like one bit of code that > validates whether a version is allowed is in the method > `reasonNotSupported` which checks the range of features available for the > given feature. > For metadata.version we have a method to do "additional checks" and we > could have those for the various other features as well. I have an > (internal) FeatureVersion interface in mind that would work well for this. > For any of these validations, we return the same error > `INVALID_UPDATE_VERSION`. I would think continuing to use this error > follows naturally, but if we think it is necessary to specify the error > code, I can do so in my KIP. Thanks for looking into this. The updates to the KIP look good to me. -- -José
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
I took a quick look at the code -- looks like the previous behavior was not to set a top level error if one particular feature had an issue. We can do that. I think it could make some sense to specify errors on features that were not valid and use the top level error to indicate that the request didn't update any features. The handling now is to complete the futures with the top level error anyway. As for the validation criteria. It seems like one bit of code that validates whether a version is allowed is in the method `reasonNotSupported` which checks the range of features available for the given feature. For metadata.version we have a method to do "additional checks" and we could have those for the various other features as well. I have an (internal) FeatureVersion interface in mind that would work well for this. For any of these validations, we return the same error `INVALID_UPDATE_VERSION`. I would think continuing to use this error follows naturally, but if we think it is necessary to specify the error code, I can do so in my KIP. Justine On Tue, Apr 9, 2024 at 1:46 PM Justine Olshan wrote: > José, > > INVALID_UPDATE_VERSION was added as part of KIP-497. The KIP seems to be > lacking some details on the error. > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-497%3A+Add+inter-broker+API+to+alter+ISR > > https://github.com/apache/kafka/commit/57de67db22eb373f92ec5dd449d317ed2bc8b8d1 > > The error seems to be used in the feature update path as well, though that > was also not included in KIP-584. I wonder if we were missing necessary > details for many KIPs in 2020... > > I'm not sure I fully understand the proposal. Is the question for the > exact error to use in UpdatableFeatureResult.ErrorCode and what to write > in UpdatableFeatureResult.ErrorMessage? If so, those errors and adding a > message (the dependency that was violated for example) makes sense. > I agree that it makes sense that any errors in updates should be a top > level error and not have a partial update. > > I thought these were part of KIP-584, but I will take a look and update > this KIP if they are not. > > Justine > > On Tue, Apr 9, 2024 at 1:10 PM José Armando García Sancio > wrote: > >> Hi Justine, >> >> Thanks for the KIP. I see that the KIP doesn't make any updates to the >> UpdateFeatures RPC. I was trying to understand how errors will be >> communicated to the client. >> >> Are you planning to use the INVALID_UPDATE_VERSION error and overwrite >> the ErrorMessage field for all of the validations you mentioned in the >> KIP? I see that INVALID_UPDATE_VERSION is in the code for Apache Kafka >> but I couldn't find the KIP that adds this error. It is not in KIP-584 >> or KIP-778. If you agree, do you think we should document this error >> in this KIP? >> >> It is also not clear to me when the UpdateFeaturesResponse will return >> an error per feature versus an error for the entire RPC. KIP-584 >> defines this relationship but it doesn't specify when exactly a top >> level error will be returned versus when a feature level error will be >> returned. I think that most users wouldn't want partial failures. They >> instead would like to be guaranteed that all of the feature updates >> succeeded or none did. Do you agree? Should we update the KIP to make >> this clear? >> >> Thanks! >> -- >> -José >> >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
José, INVALID_UPDATE_VERSION was added as part of KIP-497. The KIP seems to be lacking some details on the error. https://cwiki.apache.org/confluence/display/KAFKA/KIP-497%3A+Add+inter-broker+API+to+alter+ISR https://github.com/apache/kafka/commit/57de67db22eb373f92ec5dd449d317ed2bc8b8d1 The error seems to be used in the feature update path as well, though that was also not included in KIP-584. I wonder if we were missing necessary details for many KIPs in 2020... I'm not sure I fully understand the proposal. Is the question for the exact error to use in UpdatableFeatureResult.ErrorCode and what to write in UpdatableFeatureResult.ErrorMessage? If so, those errors and adding a message (the dependency that was violated for example) makes sense. I agree that it makes sense that any errors in updates should be a top level error and not have a partial update. I thought these were part of KIP-584, but I will take a look and update this KIP if they are not. Justine On Tue, Apr 9, 2024 at 1:10 PM José Armando García Sancio wrote: > Hi Justine, > > Thanks for the KIP. I see that the KIP doesn't make any updates to the > UpdateFeatures RPC. I was trying to understand how errors will be > communicated to the client. > > Are you planning to use the INVALID_UPDATE_VERSION error and overwrite > the ErrorMessage field for all of the validations you mentioned in the > KIP? I see that INVALID_UPDATE_VERSION is in the code for Apache Kafka > but I couldn't find the KIP that adds this error. It is not in KIP-584 > or KIP-778. If you agree, do you think we should document this error > in this KIP? > > It is also not clear to me when the UpdateFeaturesResponse will return > an error per feature versus an error for the entire RPC. KIP-584 > defines this relationship but it doesn't specify when exactly a top > level error will be returned versus when a feature level error will be > returned. I think that most users wouldn't want partial failures. They > instead would like to be guaranteed that all of the feature updates > succeeded or none did. Do you agree? Should we update the KIP to make > this clear? > > Thanks! > -- > -José >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi Justine, Thanks for the KIP. I see that the KIP doesn't make any updates to the UpdateFeatures RPC. I was trying to understand how errors will be communicated to the client. Are you planning to use the INVALID_UPDATE_VERSION error and overwrite the ErrorMessage field for all of the validations you mentioned in the KIP? I see that INVALID_UPDATE_VERSION is in the code for Apache Kafka but I couldn't find the KIP that adds this error. It is not in KIP-584 or KIP-778. If you agree, do you think we should document this error in this KIP? It is also not clear to me when the UpdateFeaturesResponse will return an error per feature versus an error for the entire RPC. KIP-584 defines this relationship but it doesn't specify when exactly a top level error will be returned versus when a feature level error will be returned. I think that most users wouldn't want partial failures. They instead would like to be guaranteed that all of the feature updates succeeded or none did. Do you agree? Should we update the KIP to make this clear? Thanks! -- -José
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi Jun, Makes sense to me. It seems like KIP-1014 has been inactive recently. I can update my KIP and mention this change on that discussion thread. Justine On Tue, Apr 9, 2024 at 9:01 AM Jun Rao wrote: > Hi, Justine, > > A single config makes sense to me too. We just need to reach consensus with > KIP-1014. > > Thanks, > > Jun > > On Mon, Apr 8, 2024 at 5:06 PM Justine Olshan > > wrote: > > > Hey Jun, > > > > That's a good question. I think maybe for simplicity, we can have a > single > > config? > > If that makes sense, I will update the KIP. > > > > Justine > > > > On Mon, Apr 8, 2024 at 3:20 PM Jun Rao wrote: > > > > > Hi, Justine, > > > > > > Thanks for the updated KIP. > > > > > > One more question related to KIP-1014. It introduced a new > > > config unstable.metadata.versions.enable. Does each new feature need to > > > have a corresponding config to enable the testing of unstable features > or > > > should we have a generic config enabling the testing of all unstable > > > features? > > > > > > Jun > > > > > > On Thu, Apr 4, 2024 at 8:24 PM Justine Olshan > > > > > > > > wrote: > > > > > > > I'm hoping this covers the majority of comments. I will go ahead and > > open > > > > the vote in the next day or so. > > > > > > > > Thanks, > > > > Justine > > > > > > > > On Wed, Apr 3, 2024 at 3:31 PM Justine Olshan > > > > wrote: > > > > > > > > > Find and replace has failed me :( > > > > > > > > > > Group version seems a little vague, but we can update it. Hopefully > > > find > > > > > and replace won't fail me again, otherwise I will get another email > > on > > > > this. > > > > > > > > > > Justine > > > > > > > > > > On Wed, Apr 3, 2024 at 12:15 PM David Jacot > > > > > > > > > > > > wrote: > > > > > > > > > >> Thanks, Justine. > > > > >> > > > > >> * Should we also use `group.version` (GV) as I suggested in my > > > previous > > > > >> message in order to be consistent? > > > > >> * Should we add both names to the `Public Interfaces` section? > > > > >> * There is still at least one usage of > > `transaction.protocol.verison` > > > in > > > > >> the KIP too. > > > > >> > > > > >> Best, > > > > >> David > > > > >> > > > > >> On Wed, Apr 3, 2024 at 6:29 PM Justine Olshan > > > > >> > > > > >> wrote: > > > > >> > > > > >> > I had missed the David's message yesterday about the naming for > > > > >> transaction > > > > >> > version vs transaction protocol version. > > > > >> > > > > > >> > After some offline discussion with Jun, Artem, and David, we > > agreed > > > > that > > > > >> > transaction version is simpler and conveys more than just > protocol > > > > >> changes > > > > >> > (flexible records for example) > > > > >> > > > > > >> > I will update the KIP as well as KIP-890 > > > > >> > > > > > >> > Thanks, > > > > >> > Justine > > > > >> > > > > > >> > On Tue, Apr 2, 2024 at 2:50 PM Justine Olshan < > > jols...@confluent.io > > > > > > > > >> > wrote: > > > > >> > > > > > >> > > Updated! > > > > >> > > > > > > >> > > Justine > > > > >> > > > > > > >> > > On Tue, Apr 2, 2024 at 2:40 PM Jun Rao > > > > > > > >> wrote: > > > > >> > > > > > > >> > >> Hi, Justine, > > > > >> > >> > > > > >> > >> Thanks for the reply. > > > > >> > >> > > > > >> > >> 21. Sounds good. It would be useful to document that. > > > > >> > >> > > > > >> > >> 22. Should we add the IV in "metadata.version=17 has no > > > > dependencies" > > > > >> > too? > > > > >> > >> > > > > >> > >> Jun > > > > >> > >> > > > > >> > >> > > > > >> > >> On Tue, Apr 2, 2024 at 11:31 AM Justine Olshan > > > > >> > >> > > > > >> > >> wrote: > > > > >> > >> > > > > >> > >> > Jun, > > > > >> > >> > > > > > >> > >> > 21. Next producer ID field doesn't need to be populated for > > TV > > > 1. > > > > >> We > > > > >> > >> don't > > > > >> > >> > have the same need to retain this since it is written > > directly > > > to > > > > >> the > > > > >> > >> > transaction log in InitProducerId. It is only needed for > > > KIP-890 > > > > >> part > > > > >> > 2 > > > > >> > >> / > > > > >> > >> > TV 2. > > > > >> > >> > > > > > >> > >> > 22. We can do that. > > > > >> > >> > > > > > >> > >> > Justine > > > > >> > >> > > > > > >> > >> > On Tue, Apr 2, 2024 at 10:41 AM Jun Rao > > > > > > > > > > > >> > >> wrote: > > > > >> > >> > > > > > >> > >> > > Hi, Justine, > > > > >> > >> > > > > > > >> > >> > > Thanks for the reply. > > > > >> > >> > > > > > > >> > >> > > 21. What about the new NextProducerId field? Will that be > > > > >> populated > > > > >> > >> with > > > > >> > >> > TV > > > > >> > >> > > 1? > > > > >> > >> > > > > > > >> > >> > > 22. In the dependencies output, should we show both IV > and > > > > level > > > > >> for > > > > >> > >> > > metadata.version too? > > > > >> > >> > > > > > > >> > >> > > Jun > > > > >> > >> > > > > > > >> > >> > > On Mon, Apr 1, 2024 at 4:43 PM Justine Olshan > > > > >> > >> > > > > >> > >> > > > > > > > >> > >> > > wrote: > > > > >> > >> > > > > > > >> > >> > > > Hi Jun, > > > > >> > >> > > > > > > > >> >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi, Justine, A single config makes sense to me too. We just need to reach consensus with KIP-1014. Thanks, Jun On Mon, Apr 8, 2024 at 5:06 PM Justine Olshan wrote: > Hey Jun, > > That's a good question. I think maybe for simplicity, we can have a single > config? > If that makes sense, I will update the KIP. > > Justine > > On Mon, Apr 8, 2024 at 3:20 PM Jun Rao wrote: > > > Hi, Justine, > > > > Thanks for the updated KIP. > > > > One more question related to KIP-1014. It introduced a new > > config unstable.metadata.versions.enable. Does each new feature need to > > have a corresponding config to enable the testing of unstable features or > > should we have a generic config enabling the testing of all unstable > > features? > > > > Jun > > > > On Thu, Apr 4, 2024 at 8:24 PM Justine Olshan > > > > > wrote: > > > > > I'm hoping this covers the majority of comments. I will go ahead and > open > > > the vote in the next day or so. > > > > > > Thanks, > > > Justine > > > > > > On Wed, Apr 3, 2024 at 3:31 PM Justine Olshan > > > wrote: > > > > > > > Find and replace has failed me :( > > > > > > > > Group version seems a little vague, but we can update it. Hopefully > > find > > > > and replace won't fail me again, otherwise I will get another email > on > > > this. > > > > > > > > Justine > > > > > > > > On Wed, Apr 3, 2024 at 12:15 PM David Jacot > > > > > > > > > wrote: > > > > > > > >> Thanks, Justine. > > > >> > > > >> * Should we also use `group.version` (GV) as I suggested in my > > previous > > > >> message in order to be consistent? > > > >> * Should we add both names to the `Public Interfaces` section? > > > >> * There is still at least one usage of > `transaction.protocol.verison` > > in > > > >> the KIP too. > > > >> > > > >> Best, > > > >> David > > > >> > > > >> On Wed, Apr 3, 2024 at 6:29 PM Justine Olshan > > > >> > > > >> wrote: > > > >> > > > >> > I had missed the David's message yesterday about the naming for > > > >> transaction > > > >> > version vs transaction protocol version. > > > >> > > > > >> > After some offline discussion with Jun, Artem, and David, we > agreed > > > that > > > >> > transaction version is simpler and conveys more than just protocol > > > >> changes > > > >> > (flexible records for example) > > > >> > > > > >> > I will update the KIP as well as KIP-890 > > > >> > > > > >> > Thanks, > > > >> > Justine > > > >> > > > > >> > On Tue, Apr 2, 2024 at 2:50 PM Justine Olshan < > jols...@confluent.io > > > > > > >> > wrote: > > > >> > > > > >> > > Updated! > > > >> > > > > > >> > > Justine > > > >> > > > > > >> > > On Tue, Apr 2, 2024 at 2:40 PM Jun Rao > > > > >> wrote: > > > >> > > > > > >> > >> Hi, Justine, > > > >> > >> > > > >> > >> Thanks for the reply. > > > >> > >> > > > >> > >> 21. Sounds good. It would be useful to document that. > > > >> > >> > > > >> > >> 22. Should we add the IV in "metadata.version=17 has no > > > dependencies" > > > >> > too? > > > >> > >> > > > >> > >> Jun > > > >> > >> > > > >> > >> > > > >> > >> On Tue, Apr 2, 2024 at 11:31 AM Justine Olshan > > > >> > >> > > > >> > >> wrote: > > > >> > >> > > > >> > >> > Jun, > > > >> > >> > > > > >> > >> > 21. Next producer ID field doesn't need to be populated for > TV > > 1. > > > >> We > > > >> > >> don't > > > >> > >> > have the same need to retain this since it is written > directly > > to > > > >> the > > > >> > >> > transaction log in InitProducerId. It is only needed for > > KIP-890 > > > >> part > > > >> > 2 > > > >> > >> / > > > >> > >> > TV 2. > > > >> > >> > > > > >> > >> > 22. We can do that. > > > >> > >> > > > > >> > >> > Justine > > > >> > >> > > > > >> > >> > On Tue, Apr 2, 2024 at 10:41 AM Jun Rao > > > > > > > > >> > >> wrote: > > > >> > >> > > > > >> > >> > > Hi, Justine, > > > >> > >> > > > > > >> > >> > > Thanks for the reply. > > > >> > >> > > > > > >> > >> > > 21. What about the new NextProducerId field? Will that be > > > >> populated > > > >> > >> with > > > >> > >> > TV > > > >> > >> > > 1? > > > >> > >> > > > > > >> > >> > > 22. In the dependencies output, should we show both IV and > > > level > > > >> for > > > >> > >> > > metadata.version too? > > > >> > >> > > > > > >> > >> > > Jun > > > >> > >> > > > > > >> > >> > > On Mon, Apr 1, 2024 at 4:43 PM Justine Olshan > > > >> > >> > > > >> > >> > > > > > > >> > >> > > wrote: > > > >> > >> > > > > > >> > >> > > > Hi Jun, > > > >> > >> > > > > > > >> > >> > > > 20. I can update the KIP. > > > >> > >> > > > > > > >> > >> > > > 21. This is used to complete some of the work with > KIP-360. > > > (We > > > >> > use > > > >> > >> > > > previous producer ID there, but never persisted it which > > was > > > in > > > >> > the > > > >> > >> KIP > > > >> > >> > > > > > > >> > >> > > > > > >> > >> > > > > >> > >> > > > >> > > > > >> > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=89068820 > > > >> > >> ) > > > >> > >> > > > The KIP also mentions including previous epoch but we > > > >> explained in
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hey Jun, That's a good question. I think maybe for simplicity, we can have a single config? If that makes sense, I will update the KIP. Justine On Mon, Apr 8, 2024 at 3:20 PM Jun Rao wrote: > Hi, Justine, > > Thanks for the updated KIP. > > One more question related to KIP-1014. It introduced a new > config unstable.metadata.versions.enable. Does each new feature need to > have a corresponding config to enable the testing of unstable features or > should we have a generic config enabling the testing of all unstable > features? > > Jun > > On Thu, Apr 4, 2024 at 8:24 PM Justine Olshan > > wrote: > > > I'm hoping this covers the majority of comments. I will go ahead and open > > the vote in the next day or so. > > > > Thanks, > > Justine > > > > On Wed, Apr 3, 2024 at 3:31 PM Justine Olshan > > wrote: > > > > > Find and replace has failed me :( > > > > > > Group version seems a little vague, but we can update it. Hopefully > find > > > and replace won't fail me again, otherwise I will get another email on > > this. > > > > > > Justine > > > > > > On Wed, Apr 3, 2024 at 12:15 PM David Jacot > > > > > > wrote: > > > > > >> Thanks, Justine. > > >> > > >> * Should we also use `group.version` (GV) as I suggested in my > previous > > >> message in order to be consistent? > > >> * Should we add both names to the `Public Interfaces` section? > > >> * There is still at least one usage of `transaction.protocol.verison` > in > > >> the KIP too. > > >> > > >> Best, > > >> David > > >> > > >> On Wed, Apr 3, 2024 at 6:29 PM Justine Olshan > > >> > > >> wrote: > > >> > > >> > I had missed the David's message yesterday about the naming for > > >> transaction > > >> > version vs transaction protocol version. > > >> > > > >> > After some offline discussion with Jun, Artem, and David, we agreed > > that > > >> > transaction version is simpler and conveys more than just protocol > > >> changes > > >> > (flexible records for example) > > >> > > > >> > I will update the KIP as well as KIP-890 > > >> > > > >> > Thanks, > > >> > Justine > > >> > > > >> > On Tue, Apr 2, 2024 at 2:50 PM Justine Olshan > > > >> > wrote: > > >> > > > >> > > Updated! > > >> > > > > >> > > Justine > > >> > > > > >> > > On Tue, Apr 2, 2024 at 2:40 PM Jun Rao > > >> wrote: > > >> > > > > >> > >> Hi, Justine, > > >> > >> > > >> > >> Thanks for the reply. > > >> > >> > > >> > >> 21. Sounds good. It would be useful to document that. > > >> > >> > > >> > >> 22. Should we add the IV in "metadata.version=17 has no > > dependencies" > > >> > too? > > >> > >> > > >> > >> Jun > > >> > >> > > >> > >> > > >> > >> On Tue, Apr 2, 2024 at 11:31 AM Justine Olshan > > >> > >> > > >> > >> wrote: > > >> > >> > > >> > >> > Jun, > > >> > >> > > > >> > >> > 21. Next producer ID field doesn't need to be populated for TV > 1. > > >> We > > >> > >> don't > > >> > >> > have the same need to retain this since it is written directly > to > > >> the > > >> > >> > transaction log in InitProducerId. It is only needed for > KIP-890 > > >> part > > >> > 2 > > >> > >> / > > >> > >> > TV 2. > > >> > >> > > > >> > >> > 22. We can do that. > > >> > >> > > > >> > >> > Justine > > >> > >> > > > >> > >> > On Tue, Apr 2, 2024 at 10:41 AM Jun Rao > > > > > >> > >> wrote: > > >> > >> > > > >> > >> > > Hi, Justine, > > >> > >> > > > > >> > >> > > Thanks for the reply. > > >> > >> > > > > >> > >> > > 21. What about the new NextProducerId field? Will that be > > >> populated > > >> > >> with > > >> > >> > TV > > >> > >> > > 1? > > >> > >> > > > > >> > >> > > 22. In the dependencies output, should we show both IV and > > level > > >> for > > >> > >> > > metadata.version too? > > >> > >> > > > > >> > >> > > Jun > > >> > >> > > > > >> > >> > > On Mon, Apr 1, 2024 at 4:43 PM Justine Olshan > > >> > >> > > >> > >> > > > > > >> > >> > > wrote: > > >> > >> > > > > >> > >> > > > Hi Jun, > > >> > >> > > > > > >> > >> > > > 20. I can update the KIP. > > >> > >> > > > > > >> > >> > > > 21. This is used to complete some of the work with KIP-360. > > (We > > >> > use > > >> > >> > > > previous producer ID there, but never persisted it which > was > > in > > >> > the > > >> > >> KIP > > >> > >> > > > > > >> > >> > > > > >> > >> > > > >> > >> > > >> > > > >> > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=89068820 > > >> > >> ) > > >> > >> > > > The KIP also mentions including previous epoch but we > > >> explained in > > >> > >> this > > >> > >> > > KIP > > >> > >> > > > how we can figure this out. > > >> > >> > > > > > >> > >> > > > Justine > > >> > >> > > > > > >> > >> > > > > > >> > >> > > > > > >> > >> > > > On Mon, Apr 1, 2024 at 3:56 PM Jun Rao > > >> > > >> > >> > wrote: > > >> > >> > > > > > >> > >> > > > > Hi, Justine, > > >> > >> > > > > > > >> > >> > > > > Thanks for the updated KIP. A couple of more comments. > > >> > >> > > > > > > >> > >> > > > > 20. Could we show the output of version-mapping? > > >> > >> > > > > > > >> > >> > > > > 21. "Transaction version 1
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi, Justine, Thanks for the updated KIP. One more question related to KIP-1014. It introduced a new config unstable.metadata.versions.enable. Does each new feature need to have a corresponding config to enable the testing of unstable features or should we have a generic config enabling the testing of all unstable features? Jun On Thu, Apr 4, 2024 at 8:24 PM Justine Olshan wrote: > I'm hoping this covers the majority of comments. I will go ahead and open > the vote in the next day or so. > > Thanks, > Justine > > On Wed, Apr 3, 2024 at 3:31 PM Justine Olshan > wrote: > > > Find and replace has failed me :( > > > > Group version seems a little vague, but we can update it. Hopefully find > > and replace won't fail me again, otherwise I will get another email on > this. > > > > Justine > > > > On Wed, Apr 3, 2024 at 12:15 PM David Jacot > > > wrote: > > > >> Thanks, Justine. > >> > >> * Should we also use `group.version` (GV) as I suggested in my previous > >> message in order to be consistent? > >> * Should we add both names to the `Public Interfaces` section? > >> * There is still at least one usage of `transaction.protocol.verison` in > >> the KIP too. > >> > >> Best, > >> David > >> > >> On Wed, Apr 3, 2024 at 6:29 PM Justine Olshan > >> > >> wrote: > >> > >> > I had missed the David's message yesterday about the naming for > >> transaction > >> > version vs transaction protocol version. > >> > > >> > After some offline discussion with Jun, Artem, and David, we agreed > that > >> > transaction version is simpler and conveys more than just protocol > >> changes > >> > (flexible records for example) > >> > > >> > I will update the KIP as well as KIP-890 > >> > > >> > Thanks, > >> > Justine > >> > > >> > On Tue, Apr 2, 2024 at 2:50 PM Justine Olshan > >> > wrote: > >> > > >> > > Updated! > >> > > > >> > > Justine > >> > > > >> > > On Tue, Apr 2, 2024 at 2:40 PM Jun Rao > >> wrote: > >> > > > >> > >> Hi, Justine, > >> > >> > >> > >> Thanks for the reply. > >> > >> > >> > >> 21. Sounds good. It would be useful to document that. > >> > >> > >> > >> 22. Should we add the IV in "metadata.version=17 has no > dependencies" > >> > too? > >> > >> > >> > >> Jun > >> > >> > >> > >> > >> > >> On Tue, Apr 2, 2024 at 11:31 AM Justine Olshan > >> > >> > >> > >> wrote: > >> > >> > >> > >> > Jun, > >> > >> > > >> > >> > 21. Next producer ID field doesn't need to be populated for TV 1. > >> We > >> > >> don't > >> > >> > have the same need to retain this since it is written directly to > >> the > >> > >> > transaction log in InitProducerId. It is only needed for KIP-890 > >> part > >> > 2 > >> > >> / > >> > >> > TV 2. > >> > >> > > >> > >> > 22. We can do that. > >> > >> > > >> > >> > Justine > >> > >> > > >> > >> > On Tue, Apr 2, 2024 at 10:41 AM Jun Rao > > >> > >> wrote: > >> > >> > > >> > >> > > Hi, Justine, > >> > >> > > > >> > >> > > Thanks for the reply. > >> > >> > > > >> > >> > > 21. What about the new NextProducerId field? Will that be > >> populated > >> > >> with > >> > >> > TV > >> > >> > > 1? > >> > >> > > > >> > >> > > 22. In the dependencies output, should we show both IV and > level > >> for > >> > >> > > metadata.version too? > >> > >> > > > >> > >> > > Jun > >> > >> > > > >> > >> > > On Mon, Apr 1, 2024 at 4:43 PM Justine Olshan > >> > >> > >> > >> > > > > >> > >> > > wrote: > >> > >> > > > >> > >> > > > Hi Jun, > >> > >> > > > > >> > >> > > > 20. I can update the KIP. > >> > >> > > > > >> > >> > > > 21. This is used to complete some of the work with KIP-360. > (We > >> > use > >> > >> > > > previous producer ID there, but never persisted it which was > in > >> > the > >> > >> KIP > >> > >> > > > > >> > >> > > > >> > >> > > >> > >> > >> > > >> > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=89068820 > >> > >> ) > >> > >> > > > The KIP also mentions including previous epoch but we > >> explained in > >> > >> this > >> > >> > > KIP > >> > >> > > > how we can figure this out. > >> > >> > > > > >> > >> > > > Justine > >> > >> > > > > >> > >> > > > > >> > >> > > > > >> > >> > > > On Mon, Apr 1, 2024 at 3:56 PM Jun Rao > >> > >> > >> > wrote: > >> > >> > > > > >> > >> > > > > Hi, Justine, > >> > >> > > > > > >> > >> > > > > Thanks for the updated KIP. A couple of more comments. > >> > >> > > > > > >> > >> > > > > 20. Could we show the output of version-mapping? > >> > >> > > > > > >> > >> > > > > 21. "Transaction version 1 will include the flexible fields > >> in > >> > the > >> > >> > > > > transaction state log, and transaction version 2 will > include > >> > the > >> > >> > > changes > >> > >> > > > > to the transactional protocol as described by KIP-890 > (epoch > >> > bumps > >> > >> > and > >> > >> > > > > implicit add partitions.)" > >> > >> > > > > So TV 1 enables the writing of new tagged fields like > >> > >> > PrevProducerId? > >> > >> > > > But > >> > >> > > > > those fields are only usable after the epoch bump, right? > >> What > >> > >> > > > > functionality does
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
I'm hoping this covers the majority of comments. I will go ahead and open the vote in the next day or so. Thanks, Justine On Wed, Apr 3, 2024 at 3:31 PM Justine Olshan wrote: > Find and replace has failed me :( > > Group version seems a little vague, but we can update it. Hopefully find > and replace won't fail me again, otherwise I will get another email on this. > > Justine > > On Wed, Apr 3, 2024 at 12:15 PM David Jacot > wrote: > >> Thanks, Justine. >> >> * Should we also use `group.version` (GV) as I suggested in my previous >> message in order to be consistent? >> * Should we add both names to the `Public Interfaces` section? >> * There is still at least one usage of `transaction.protocol.verison` in >> the KIP too. >> >> Best, >> David >> >> On Wed, Apr 3, 2024 at 6:29 PM Justine Olshan >> >> wrote: >> >> > I had missed the David's message yesterday about the naming for >> transaction >> > version vs transaction protocol version. >> > >> > After some offline discussion with Jun, Artem, and David, we agreed that >> > transaction version is simpler and conveys more than just protocol >> changes >> > (flexible records for example) >> > >> > I will update the KIP as well as KIP-890 >> > >> > Thanks, >> > Justine >> > >> > On Tue, Apr 2, 2024 at 2:50 PM Justine Olshan >> > wrote: >> > >> > > Updated! >> > > >> > > Justine >> > > >> > > On Tue, Apr 2, 2024 at 2:40 PM Jun Rao >> wrote: >> > > >> > >> Hi, Justine, >> > >> >> > >> Thanks for the reply. >> > >> >> > >> 21. Sounds good. It would be useful to document that. >> > >> >> > >> 22. Should we add the IV in "metadata.version=17 has no dependencies" >> > too? >> > >> >> > >> Jun >> > >> >> > >> >> > >> On Tue, Apr 2, 2024 at 11:31 AM Justine Olshan >> > >> >> > >> wrote: >> > >> >> > >> > Jun, >> > >> > >> > >> > 21. Next producer ID field doesn't need to be populated for TV 1. >> We >> > >> don't >> > >> > have the same need to retain this since it is written directly to >> the >> > >> > transaction log in InitProducerId. It is only needed for KIP-890 >> part >> > 2 >> > >> / >> > >> > TV 2. >> > >> > >> > >> > 22. We can do that. >> > >> > >> > >> > Justine >> > >> > >> > >> > On Tue, Apr 2, 2024 at 10:41 AM Jun Rao >> > >> wrote: >> > >> > >> > >> > > Hi, Justine, >> > >> > > >> > >> > > Thanks for the reply. >> > >> > > >> > >> > > 21. What about the new NextProducerId field? Will that be >> populated >> > >> with >> > >> > TV >> > >> > > 1? >> > >> > > >> > >> > > 22. In the dependencies output, should we show both IV and level >> for >> > >> > > metadata.version too? >> > >> > > >> > >> > > Jun >> > >> > > >> > >> > > On Mon, Apr 1, 2024 at 4:43 PM Justine Olshan >> > >> > > > >> > > > >> > >> > > wrote: >> > >> > > >> > >> > > > Hi Jun, >> > >> > > > >> > >> > > > 20. I can update the KIP. >> > >> > > > >> > >> > > > 21. This is used to complete some of the work with KIP-360. (We >> > use >> > >> > > > previous producer ID there, but never persisted it which was in >> > the >> > >> KIP >> > >> > > > >> > >> > > >> > >> > >> > >> >> > >> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=89068820 >> > >> ) >> > >> > > > The KIP also mentions including previous epoch but we >> explained in >> > >> this >> > >> > > KIP >> > >> > > > how we can figure this out. >> > >> > > > >> > >> > > > Justine >> > >> > > > >> > >> > > > >> > >> > > > >> > >> > > > On Mon, Apr 1, 2024 at 3:56 PM Jun Rao >> >> > >> > wrote: >> > >> > > > >> > >> > > > > Hi, Justine, >> > >> > > > > >> > >> > > > > Thanks for the updated KIP. A couple of more comments. >> > >> > > > > >> > >> > > > > 20. Could we show the output of version-mapping? >> > >> > > > > >> > >> > > > > 21. "Transaction version 1 will include the flexible fields >> in >> > the >> > >> > > > > transaction state log, and transaction version 2 will include >> > the >> > >> > > changes >> > >> > > > > to the transactional protocol as described by KIP-890 (epoch >> > bumps >> > >> > and >> > >> > > > > implicit add partitions.)" >> > >> > > > > So TV 1 enables the writing of new tagged fields like >> > >> > PrevProducerId? >> > >> > > > But >> > >> > > > > those fields are only usable after the epoch bump, right? >> What >> > >> > > > > functionality does TV 1 achieve? >> > >> > > > > >> > >> > > > > Jun >> > >> > > > > >> > >> > > > > >> > >> > > > > On Mon, Apr 1, 2024 at 2:06 PM Justine Olshan >> > >> > > > > > >> > > > > > >> > >> > > > > wrote: >> > >> > > > > >> > >> > > > > > I have also updated the KIP to mention the feature tool's >> > >> > --metadata >> > >> > > > flag >> > >> > > > > > will be deprecated. >> > >> > > > > > It will still work for users as they learn the new flag, >> but a >> > >> > > warning >> > >> > > > > > indicating the alternatives will be shown. >> > >> > > > > > >> > >> > > > > > Justine >> > >> > > > > > >> > >> > > > > > On Thu, Mar 28, 2024 at 11:03 AM Justine Olshan < >> > >> > > jols...@confluent.io> >> > >> > > > > > wrote: >> > >> > > > > >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Find and replace has failed me :( Group version seems a little vague, but we can update it. Hopefully find and replace won't fail me again, otherwise I will get another email on this. Justine On Wed, Apr 3, 2024 at 12:15 PM David Jacot wrote: > Thanks, Justine. > > * Should we also use `group.version` (GV) as I suggested in my previous > message in order to be consistent? > * Should we add both names to the `Public Interfaces` section? > * There is still at least one usage of `transaction.protocol.verison` in > the KIP too. > > Best, > David > > On Wed, Apr 3, 2024 at 6:29 PM Justine Olshan > > wrote: > > > I had missed the David's message yesterday about the naming for > transaction > > version vs transaction protocol version. > > > > After some offline discussion with Jun, Artem, and David, we agreed that > > transaction version is simpler and conveys more than just protocol > changes > > (flexible records for example) > > > > I will update the KIP as well as KIP-890 > > > > Thanks, > > Justine > > > > On Tue, Apr 2, 2024 at 2:50 PM Justine Olshan > > wrote: > > > > > Updated! > > > > > > Justine > > > > > > On Tue, Apr 2, 2024 at 2:40 PM Jun Rao > wrote: > > > > > >> Hi, Justine, > > >> > > >> Thanks for the reply. > > >> > > >> 21. Sounds good. It would be useful to document that. > > >> > > >> 22. Should we add the IV in "metadata.version=17 has no dependencies" > > too? > > >> > > >> Jun > > >> > > >> > > >> On Tue, Apr 2, 2024 at 11:31 AM Justine Olshan > > >> > > >> wrote: > > >> > > >> > Jun, > > >> > > > >> > 21. Next producer ID field doesn't need to be populated for TV 1. We > > >> don't > > >> > have the same need to retain this since it is written directly to > the > > >> > transaction log in InitProducerId. It is only needed for KIP-890 > part > > 2 > > >> / > > >> > TV 2. > > >> > > > >> > 22. We can do that. > > >> > > > >> > Justine > > >> > > > >> > On Tue, Apr 2, 2024 at 10:41 AM Jun Rao > > >> wrote: > > >> > > > >> > > Hi, Justine, > > >> > > > > >> > > Thanks for the reply. > > >> > > > > >> > > 21. What about the new NextProducerId field? Will that be > populated > > >> with > > >> > TV > > >> > > 1? > > >> > > > > >> > > 22. In the dependencies output, should we show both IV and level > for > > >> > > metadata.version too? > > >> > > > > >> > > Jun > > >> > > > > >> > > On Mon, Apr 1, 2024 at 4:43 PM Justine Olshan > > >> > > >> > > > > > >> > > wrote: > > >> > > > > >> > > > Hi Jun, > > >> > > > > > >> > > > 20. I can update the KIP. > > >> > > > > > >> > > > 21. This is used to complete some of the work with KIP-360. (We > > use > > >> > > > previous producer ID there, but never persisted it which was in > > the > > >> KIP > > >> > > > > > >> > > > > >> > > > >> > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=89068820 > > >> ) > > >> > > > The KIP also mentions including previous epoch but we explained > in > > >> this > > >> > > KIP > > >> > > > how we can figure this out. > > >> > > > > > >> > > > Justine > > >> > > > > > >> > > > > > >> > > > > > >> > > > On Mon, Apr 1, 2024 at 3:56 PM Jun Rao > > > >> > wrote: > > >> > > > > > >> > > > > Hi, Justine, > > >> > > > > > > >> > > > > Thanks for the updated KIP. A couple of more comments. > > >> > > > > > > >> > > > > 20. Could we show the output of version-mapping? > > >> > > > > > > >> > > > > 21. "Transaction version 1 will include the flexible fields in > > the > > >> > > > > transaction state log, and transaction version 2 will include > > the > > >> > > changes > > >> > > > > to the transactional protocol as described by KIP-890 (epoch > > bumps > > >> > and > > >> > > > > implicit add partitions.)" > > >> > > > > So TV 1 enables the writing of new tagged fields like > > >> > PrevProducerId? > > >> > > > But > > >> > > > > those fields are only usable after the epoch bump, right? What > > >> > > > > functionality does TV 1 achieve? > > >> > > > > > > >> > > > > Jun > > >> > > > > > > >> > > > > > > >> > > > > On Mon, Apr 1, 2024 at 2:06 PM Justine Olshan > > >> > > > > >> > > > > > > > >> > > > > wrote: > > >> > > > > > > >> > > > > > I have also updated the KIP to mention the feature tool's > > >> > --metadata > > >> > > > flag > > >> > > > > > will be deprecated. > > >> > > > > > It will still work for users as they learn the new flag, > but a > > >> > > warning > > >> > > > > > indicating the alternatives will be shown. > > >> > > > > > > > >> > > > > > Justine > > >> > > > > > > > >> > > > > > On Thu, Mar 28, 2024 at 11:03 AM Justine Olshan < > > >> > > jols...@confluent.io> > > >> > > > > > wrote: > > >> > > > > > > > >> > > > > > > Hi Jun, > > >> > > > > > > > > >> > > > > > > For both transaction state and group coordinator state, > > there > > >> are > > >> > > > only > > >> > > > > > > version 0 records. > > >> > > > > > > KIP-915 introduced flexible versions, but it was never put > > to > > >> > use. > > >> > > MV > > >> > > > > has > > >> > > > > > > never gated these. This
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Thanks, Justine. * Should we also use `group.version` (GV) as I suggested in my previous message in order to be consistent? * Should we add both names to the `Public Interfaces` section? * There is still at least one usage of `transaction.protocol.verison` in the KIP too. Best, David On Wed, Apr 3, 2024 at 6:29 PM Justine Olshan wrote: > I had missed the David's message yesterday about the naming for transaction > version vs transaction protocol version. > > After some offline discussion with Jun, Artem, and David, we agreed that > transaction version is simpler and conveys more than just protocol changes > (flexible records for example) > > I will update the KIP as well as KIP-890 > > Thanks, > Justine > > On Tue, Apr 2, 2024 at 2:50 PM Justine Olshan > wrote: > > > Updated! > > > > Justine > > > > On Tue, Apr 2, 2024 at 2:40 PM Jun Rao wrote: > > > >> Hi, Justine, > >> > >> Thanks for the reply. > >> > >> 21. Sounds good. It would be useful to document that. > >> > >> 22. Should we add the IV in "metadata.version=17 has no dependencies" > too? > >> > >> Jun > >> > >> > >> On Tue, Apr 2, 2024 at 11:31 AM Justine Olshan > >> > >> wrote: > >> > >> > Jun, > >> > > >> > 21. Next producer ID field doesn't need to be populated for TV 1. We > >> don't > >> > have the same need to retain this since it is written directly to the > >> > transaction log in InitProducerId. It is only needed for KIP-890 part > 2 > >> / > >> > TV 2. > >> > > >> > 22. We can do that. > >> > > >> > Justine > >> > > >> > On Tue, Apr 2, 2024 at 10:41 AM Jun Rao > >> wrote: > >> > > >> > > Hi, Justine, > >> > > > >> > > Thanks for the reply. > >> > > > >> > > 21. What about the new NextProducerId field? Will that be populated > >> with > >> > TV > >> > > 1? > >> > > > >> > > 22. In the dependencies output, should we show both IV and level for > >> > > metadata.version too? > >> > > > >> > > Jun > >> > > > >> > > On Mon, Apr 1, 2024 at 4:43 PM Justine Olshan > >> > >> > > > > >> > > wrote: > >> > > > >> > > > Hi Jun, > >> > > > > >> > > > 20. I can update the KIP. > >> > > > > >> > > > 21. This is used to complete some of the work with KIP-360. (We > use > >> > > > previous producer ID there, but never persisted it which was in > the > >> KIP > >> > > > > >> > > > >> > > >> > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=89068820 > >> ) > >> > > > The KIP also mentions including previous epoch but we explained in > >> this > >> > > KIP > >> > > > how we can figure this out. > >> > > > > >> > > > Justine > >> > > > > >> > > > > >> > > > > >> > > > On Mon, Apr 1, 2024 at 3:56 PM Jun Rao > >> > wrote: > >> > > > > >> > > > > Hi, Justine, > >> > > > > > >> > > > > Thanks for the updated KIP. A couple of more comments. > >> > > > > > >> > > > > 20. Could we show the output of version-mapping? > >> > > > > > >> > > > > 21. "Transaction version 1 will include the flexible fields in > the > >> > > > > transaction state log, and transaction version 2 will include > the > >> > > changes > >> > > > > to the transactional protocol as described by KIP-890 (epoch > bumps > >> > and > >> > > > > implicit add partitions.)" > >> > > > > So TV 1 enables the writing of new tagged fields like > >> > PrevProducerId? > >> > > > But > >> > > > > those fields are only usable after the epoch bump, right? What > >> > > > > functionality does TV 1 achieve? > >> > > > > > >> > > > > Jun > >> > > > > > >> > > > > > >> > > > > On Mon, Apr 1, 2024 at 2:06 PM Justine Olshan > >> > > > >> > > > > > > >> > > > > wrote: > >> > > > > > >> > > > > > I have also updated the KIP to mention the feature tool's > >> > --metadata > >> > > > flag > >> > > > > > will be deprecated. > >> > > > > > It will still work for users as they learn the new flag, but a > >> > > warning > >> > > > > > indicating the alternatives will be shown. > >> > > > > > > >> > > > > > Justine > >> > > > > > > >> > > > > > On Thu, Mar 28, 2024 at 11:03 AM Justine Olshan < > >> > > jols...@confluent.io> > >> > > > > > wrote: > >> > > > > > > >> > > > > > > Hi Jun, > >> > > > > > > > >> > > > > > > For both transaction state and group coordinator state, > there > >> are > >> > > > only > >> > > > > > > version 0 records. > >> > > > > > > KIP-915 introduced flexible versions, but it was never put > to > >> > use. > >> > > MV > >> > > > > has > >> > > > > > > never gated these. This KIP will do that. I can include this > >> > > context > >> > > > in > >> > > > > > the > >> > > > > > > KIP. > >> > > > > > > > >> > > > > > > I'm happy to modify his 1 and 2 to 0 and 1. > >> > > > > > > > >> > > > > > > Justine > >> > > > > > > > >> > > > > > > On Thu, Mar 28, 2024 at 10:57 AM Jun Rao > >> > >> > > > > >> > > > > > wrote: > >> > > > > > > > >> > > > > > >> Hi, David, > >> > > > > > >> > >> > > > > > >> Thanks for the reply. > >> > > > > > >> > >> > > > > > >> Historically, the format of all records were controlled by > >> MV. > >> > > Now, > >> > > > > > >> records > >> > > > > > >>
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
I had missed the David's message yesterday about the naming for transaction version vs transaction protocol version. After some offline discussion with Jun, Artem, and David, we agreed that transaction version is simpler and conveys more than just protocol changes (flexible records for example) I will update the KIP as well as KIP-890 Thanks, Justine On Tue, Apr 2, 2024 at 2:50 PM Justine Olshan wrote: > Updated! > > Justine > > On Tue, Apr 2, 2024 at 2:40 PM Jun Rao wrote: > >> Hi, Justine, >> >> Thanks for the reply. >> >> 21. Sounds good. It would be useful to document that. >> >> 22. Should we add the IV in "metadata.version=17 has no dependencies" too? >> >> Jun >> >> >> On Tue, Apr 2, 2024 at 11:31 AM Justine Olshan >> >> wrote: >> >> > Jun, >> > >> > 21. Next producer ID field doesn't need to be populated for TV 1. We >> don't >> > have the same need to retain this since it is written directly to the >> > transaction log in InitProducerId. It is only needed for KIP-890 part 2 >> / >> > TV 2. >> > >> > 22. We can do that. >> > >> > Justine >> > >> > On Tue, Apr 2, 2024 at 10:41 AM Jun Rao >> wrote: >> > >> > > Hi, Justine, >> > > >> > > Thanks for the reply. >> > > >> > > 21. What about the new NextProducerId field? Will that be populated >> with >> > TV >> > > 1? >> > > >> > > 22. In the dependencies output, should we show both IV and level for >> > > metadata.version too? >> > > >> > > Jun >> > > >> > > On Mon, Apr 1, 2024 at 4:43 PM Justine Olshan >> > > > > > >> > > wrote: >> > > >> > > > Hi Jun, >> > > > >> > > > 20. I can update the KIP. >> > > > >> > > > 21. This is used to complete some of the work with KIP-360. (We use >> > > > previous producer ID there, but never persisted it which was in the >> KIP >> > > > >> > > >> > >> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=89068820 >> ) >> > > > The KIP also mentions including previous epoch but we explained in >> this >> > > KIP >> > > > how we can figure this out. >> > > > >> > > > Justine >> > > > >> > > > >> > > > >> > > > On Mon, Apr 1, 2024 at 3:56 PM Jun Rao >> > wrote: >> > > > >> > > > > Hi, Justine, >> > > > > >> > > > > Thanks for the updated KIP. A couple of more comments. >> > > > > >> > > > > 20. Could we show the output of version-mapping? >> > > > > >> > > > > 21. "Transaction version 1 will include the flexible fields in the >> > > > > transaction state log, and transaction version 2 will include the >> > > changes >> > > > > to the transactional protocol as described by KIP-890 (epoch bumps >> > and >> > > > > implicit add partitions.)" >> > > > > So TV 1 enables the writing of new tagged fields like >> > PrevProducerId? >> > > > But >> > > > > those fields are only usable after the epoch bump, right? What >> > > > > functionality does TV 1 achieve? >> > > > > >> > > > > Jun >> > > > > >> > > > > >> > > > > On Mon, Apr 1, 2024 at 2:06 PM Justine Olshan >> > > > > > > > > > >> > > > > wrote: >> > > > > >> > > > > > I have also updated the KIP to mention the feature tool's >> > --metadata >> > > > flag >> > > > > > will be deprecated. >> > > > > > It will still work for users as they learn the new flag, but a >> > > warning >> > > > > > indicating the alternatives will be shown. >> > > > > > >> > > > > > Justine >> > > > > > >> > > > > > On Thu, Mar 28, 2024 at 11:03 AM Justine Olshan < >> > > jols...@confluent.io> >> > > > > > wrote: >> > > > > > >> > > > > > > Hi Jun, >> > > > > > > >> > > > > > > For both transaction state and group coordinator state, there >> are >> > > > only >> > > > > > > version 0 records. >> > > > > > > KIP-915 introduced flexible versions, but it was never put to >> > use. >> > > MV >> > > > > has >> > > > > > > never gated these. This KIP will do that. I can include this >> > > context >> > > > in >> > > > > > the >> > > > > > > KIP. >> > > > > > > >> > > > > > > I'm happy to modify his 1 and 2 to 0 and 1. >> > > > > > > >> > > > > > > Justine >> > > > > > > >> > > > > > > On Thu, Mar 28, 2024 at 10:57 AM Jun Rao >> > > > > > >> > > > > > wrote: >> > > > > > > >> > > > > > >> Hi, David, >> > > > > > >> >> > > > > > >> Thanks for the reply. >> > > > > > >> >> > > > > > >> Historically, the format of all records were controlled by >> MV. >> > > Now, >> > > > > > >> records >> > > > > > >> in _offset_commit will be controlled by >> > > `group.coordinator.version`, >> > > > > is >> > > > > > >> that right? It would be useful to document that. >> > > > > > >> >> > > > > > >> Also, we should align on the version numbering. >> "kafka-feature >> > > > > disable" >> > > > > > >> says "Disable one or more feature flags. This is the same as >> > > > > downgrading >> > > > > > >> the version to zero". So, in the `group.coordinator.version' >> > case, >> > > > we >> > > > > > >> probably should use version 0 for the old consumer protocol. >> > > > > > >> >> > > > > > >> Jun >> > > > > > >> >> > > > > > >> On Thu, Mar 28, 2024 at 2:13 AM Andrew Schofield < >> > > > > > >>
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Updated! Justine On Tue, Apr 2, 2024 at 2:40 PM Jun Rao wrote: > Hi, Justine, > > Thanks for the reply. > > 21. Sounds good. It would be useful to document that. > > 22. Should we add the IV in "metadata.version=17 has no dependencies" too? > > Jun > > > On Tue, Apr 2, 2024 at 11:31 AM Justine Olshan > > wrote: > > > Jun, > > > > 21. Next producer ID field doesn't need to be populated for TV 1. We > don't > > have the same need to retain this since it is written directly to the > > transaction log in InitProducerId. It is only needed for KIP-890 part 2 / > > TV 2. > > > > 22. We can do that. > > > > Justine > > > > On Tue, Apr 2, 2024 at 10:41 AM Jun Rao > wrote: > > > > > Hi, Justine, > > > > > > Thanks for the reply. > > > > > > 21. What about the new NextProducerId field? Will that be populated > with > > TV > > > 1? > > > > > > 22. In the dependencies output, should we show both IV and level for > > > metadata.version too? > > > > > > Jun > > > > > > On Mon, Apr 1, 2024 at 4:43 PM Justine Olshan > > > > > > > > wrote: > > > > > > > Hi Jun, > > > > > > > > 20. I can update the KIP. > > > > > > > > 21. This is used to complete some of the work with KIP-360. (We use > > > > previous producer ID there, but never persisted it which was in the > KIP > > > > > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=89068820) > > > > The KIP also mentions including previous epoch but we explained in > this > > > KIP > > > > how we can figure this out. > > > > > > > > Justine > > > > > > > > > > > > > > > > On Mon, Apr 1, 2024 at 3:56 PM Jun Rao > > wrote: > > > > > > > > > Hi, Justine, > > > > > > > > > > Thanks for the updated KIP. A couple of more comments. > > > > > > > > > > 20. Could we show the output of version-mapping? > > > > > > > > > > 21. "Transaction version 1 will include the flexible fields in the > > > > > transaction state log, and transaction version 2 will include the > > > changes > > > > > to the transactional protocol as described by KIP-890 (epoch bumps > > and > > > > > implicit add partitions.)" > > > > > So TV 1 enables the writing of new tagged fields like > > PrevProducerId? > > > > But > > > > > those fields are only usable after the epoch bump, right? What > > > > > functionality does TV 1 achieve? > > > > > > > > > > Jun > > > > > > > > > > > > > > > On Mon, Apr 1, 2024 at 2:06 PM Justine Olshan > > > > > > > > > > > > > > wrote: > > > > > > > > > > > I have also updated the KIP to mention the feature tool's > > --metadata > > > > flag > > > > > > will be deprecated. > > > > > > It will still work for users as they learn the new flag, but a > > > warning > > > > > > indicating the alternatives will be shown. > > > > > > > > > > > > Justine > > > > > > > > > > > > On Thu, Mar 28, 2024 at 11:03 AM Justine Olshan < > > > jols...@confluent.io> > > > > > > wrote: > > > > > > > > > > > > > Hi Jun, > > > > > > > > > > > > > > For both transaction state and group coordinator state, there > are > > > > only > > > > > > > version 0 records. > > > > > > > KIP-915 introduced flexible versions, but it was never put to > > use. > > > MV > > > > > has > > > > > > > never gated these. This KIP will do that. I can include this > > > context > > > > in > > > > > > the > > > > > > > KIP. > > > > > > > > > > > > > > I'm happy to modify his 1 and 2 to 0 and 1. > > > > > > > > > > > > > > Justine > > > > > > > > > > > > > > On Thu, Mar 28, 2024 at 10:57 AM Jun Rao > > > > > > > > > > > wrote: > > > > > > > > > > > > > >> Hi, David, > > > > > > >> > > > > > > >> Thanks for the reply. > > > > > > >> > > > > > > >> Historically, the format of all records were controlled by MV. > > > Now, > > > > > > >> records > > > > > > >> in _offset_commit will be controlled by > > > `group.coordinator.version`, > > > > > is > > > > > > >> that right? It would be useful to document that. > > > > > > >> > > > > > > >> Also, we should align on the version numbering. "kafka-feature > > > > > disable" > > > > > > >> says "Disable one or more feature flags. This is the same as > > > > > downgrading > > > > > > >> the version to zero". So, in the `group.coordinator.version' > > case, > > > > we > > > > > > >> probably should use version 0 for the old consumer protocol. > > > > > > >> > > > > > > >> Jun > > > > > > >> > > > > > > >> On Thu, Mar 28, 2024 at 2:13 AM Andrew Schofield < > > > > > > >> andrew_schofield_j...@outlook.com> wrote: > > > > > > >> > > > > > > >> > Hi David, > > > > > > >> > I agree that we should use the same mechanism to gate > KIP-932 > > > once > > > > > > that > > > > > > >> > feature reaches production readiness. The precise details of > > the > > > > > > values > > > > > > >> > will > > > > > > >> > depend upon the current state of all these flags when that > > > release > > > > > > >> comes. > > > > > > >> > > > > > > > >> > Thanks, > > > > > > >> > Andrew > > > > > > >> > > > > > > > >> > > On 28 Mar 2024, at 07:11, David Jacot > > > > > > > > > > > > > > > >> >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi, Justine, Thanks for the reply. 21. Sounds good. It would be useful to document that. 22. Should we add the IV in "metadata.version=17 has no dependencies" too? Jun On Tue, Apr 2, 2024 at 11:31 AM Justine Olshan wrote: > Jun, > > 21. Next producer ID field doesn't need to be populated for TV 1. We don't > have the same need to retain this since it is written directly to the > transaction log in InitProducerId. It is only needed for KIP-890 part 2 / > TV 2. > > 22. We can do that. > > Justine > > On Tue, Apr 2, 2024 at 10:41 AM Jun Rao wrote: > > > Hi, Justine, > > > > Thanks for the reply. > > > > 21. What about the new NextProducerId field? Will that be populated with > TV > > 1? > > > > 22. In the dependencies output, should we show both IV and level for > > metadata.version too? > > > > Jun > > > > On Mon, Apr 1, 2024 at 4:43 PM Justine Olshan > > > > > wrote: > > > > > Hi Jun, > > > > > > 20. I can update the KIP. > > > > > > 21. This is used to complete some of the work with KIP-360. (We use > > > previous producer ID there, but never persisted it which was in the KIP > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=89068820) > > > The KIP also mentions including previous epoch but we explained in this > > KIP > > > how we can figure this out. > > > > > > Justine > > > > > > > > > > > > On Mon, Apr 1, 2024 at 3:56 PM Jun Rao > wrote: > > > > > > > Hi, Justine, > > > > > > > > Thanks for the updated KIP. A couple of more comments. > > > > > > > > 20. Could we show the output of version-mapping? > > > > > > > > 21. "Transaction version 1 will include the flexible fields in the > > > > transaction state log, and transaction version 2 will include the > > changes > > > > to the transactional protocol as described by KIP-890 (epoch bumps > and > > > > implicit add partitions.)" > > > > So TV 1 enables the writing of new tagged fields like > PrevProducerId? > > > But > > > > those fields are only usable after the epoch bump, right? What > > > > functionality does TV 1 achieve? > > > > > > > > Jun > > > > > > > > > > > > On Mon, Apr 1, 2024 at 2:06 PM Justine Olshan > > > > > > > > > > > wrote: > > > > > > > > > I have also updated the KIP to mention the feature tool's > --metadata > > > flag > > > > > will be deprecated. > > > > > It will still work for users as they learn the new flag, but a > > warning > > > > > indicating the alternatives will be shown. > > > > > > > > > > Justine > > > > > > > > > > On Thu, Mar 28, 2024 at 11:03 AM Justine Olshan < > > jols...@confluent.io> > > > > > wrote: > > > > > > > > > > > Hi Jun, > > > > > > > > > > > > For both transaction state and group coordinator state, there are > > > only > > > > > > version 0 records. > > > > > > KIP-915 introduced flexible versions, but it was never put to > use. > > MV > > > > has > > > > > > never gated these. This KIP will do that. I can include this > > context > > > in > > > > > the > > > > > > KIP. > > > > > > > > > > > > I'm happy to modify his 1 and 2 to 0 and 1. > > > > > > > > > > > > Justine > > > > > > > > > > > > On Thu, Mar 28, 2024 at 10:57 AM Jun Rao > > > > > > > > wrote: > > > > > > > > > > > >> Hi, David, > > > > > >> > > > > > >> Thanks for the reply. > > > > > >> > > > > > >> Historically, the format of all records were controlled by MV. > > Now, > > > > > >> records > > > > > >> in _offset_commit will be controlled by > > `group.coordinator.version`, > > > > is > > > > > >> that right? It would be useful to document that. > > > > > >> > > > > > >> Also, we should align on the version numbering. "kafka-feature > > > > disable" > > > > > >> says "Disable one or more feature flags. This is the same as > > > > downgrading > > > > > >> the version to zero". So, in the `group.coordinator.version' > case, > > > we > > > > > >> probably should use version 0 for the old consumer protocol. > > > > > >> > > > > > >> Jun > > > > > >> > > > > > >> On Thu, Mar 28, 2024 at 2:13 AM Andrew Schofield < > > > > > >> andrew_schofield_j...@outlook.com> wrote: > > > > > >> > > > > > >> > Hi David, > > > > > >> > I agree that we should use the same mechanism to gate KIP-932 > > once > > > > > that > > > > > >> > feature reaches production readiness. The precise details of > the > > > > > values > > > > > >> > will > > > > > >> > depend upon the current state of all these flags when that > > release > > > > > >> comes. > > > > > >> > > > > > > >> > Thanks, > > > > > >> > Andrew > > > > > >> > > > > > > >> > > On 28 Mar 2024, at 07:11, David Jacot > > > > > > > > > > > > >> > wrote: > > > > > >> > > > > > > > >> > > Hi, Jun, Justine, > > > > > >> > > > > > > > >> > > Regarding `group.coordinator.version`, the idea is to use it > > to > > > > gate > > > > > >> > > records and APIs of the group coordinator. The first use > case > > > will > > > > > be > > > > > >> > > KIP-848. We will use version 2 of the flag to gate all the > new > > > > > records > > > > > >> > and > > > > > >> > > the new
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi José I originally had each on a new line and then switched to a single line since it looked like a lot of space. I can switch it back. I don't think it makes a big difference if we implement it for both tools. Both tools will have use for it. I can change the name to feature-dependencies if that makes it clearer. I can say that we won't allow cycles, but I'm not sure the best way to enforce this. I just put ">" to show output. I can change the color if that makes it clearer. Justine On Tue, Apr 2, 2024 at 11:41 AM José Armando García Sancio wrote: > Hi Justine, > > See my comments below. > > On Mon, Apr 1, 2024 at 4:43 PM Justine Olshan > wrote: > > 20. I can update the KIP. > > I took a look at the latest KIP. > > * Should the output of this command "bin/kafka-features.sh > version-mapping --release-version 3.6-IV1" be something like this: > metadata.version=13 > transaction.protocol.version=0 > group.coordinator.version=0 > kraft.version=0 > > I added the kraft.version to show the changes that are coming from > KIP-853. I find this formatting much easier to parse by scripts/tools. > They can even use Java's Properties parser if they want. > > * Maybe I missed the discussion but can you talk about why both > "kafka-storage" and "kafka-features" are going to implement the > "version-mapping" and dependencies"? I assume that it is sufficient > for only one (kafka-features) to implement these new commands. > > * For the command "dependencies" in the "kafka-features" command, it > is probably obvious that the dependencies listed are feature version > dependencies. This is not obvious when the user uses "kafka-storage > dependencies". This reads as the dependencies of kafka-storage. > > * Should we state that Kafka will not allow cycles in the feature > version dependency definition? Meaning the user is not allowed to > define a feature version X which depends on Y which depends on Z which > depends on X. > > * Some of the example output start with the characters "> ". Will the > CLI print those characters or is that just part of the KIP's > formating? > > Thanks, > -- > -José >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi Justine, See my comments below. On Mon, Apr 1, 2024 at 4:43 PM Justine Olshan wrote: > 20. I can update the KIP. I took a look at the latest KIP. * Should the output of this command "bin/kafka-features.sh version-mapping --release-version 3.6-IV1" be something like this: metadata.version=13 transaction.protocol.version=0 group.coordinator.version=0 kraft.version=0 I added the kraft.version to show the changes that are coming from KIP-853. I find this formatting much easier to parse by scripts/tools. They can even use Java's Properties parser if they want. * Maybe I missed the discussion but can you talk about why both "kafka-storage" and "kafka-features" are going to implement the "version-mapping" and dependencies"? I assume that it is sufficient for only one (kafka-features) to implement these new commands. * For the command "dependencies" in the "kafka-features" command, it is probably obvious that the dependencies listed are feature version dependencies. This is not obvious when the user uses "kafka-storage dependencies". This reads as the dependencies of kafka-storage. * Should we state that Kafka will not allow cycles in the feature version dependency definition? Meaning the user is not allowed to define a feature version X which depends on Y which depends on Z which depends on X. * Some of the example output start with the characters "> ". Will the CLI print those characters or is that just part of the KIP's formating? Thanks, -- -José
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Jun, 21. Next producer ID field doesn't need to be populated for TV 1. We don't have the same need to retain this since it is written directly to the transaction log in InitProducerId. It is only needed for KIP-890 part 2 / TV 2. 22. We can do that. Justine On Tue, Apr 2, 2024 at 10:41 AM Jun Rao wrote: > Hi, Justine, > > Thanks for the reply. > > 21. What about the new NextProducerId field? Will that be populated with TV > 1? > > 22. In the dependencies output, should we show both IV and level for > metadata.version too? > > Jun > > On Mon, Apr 1, 2024 at 4:43 PM Justine Olshan > > wrote: > > > Hi Jun, > > > > 20. I can update the KIP. > > > > 21. This is used to complete some of the work with KIP-360. (We use > > previous producer ID there, but never persisted it which was in the KIP > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=89068820) > > The KIP also mentions including previous epoch but we explained in this > KIP > > how we can figure this out. > > > > Justine > > > > > > > > On Mon, Apr 1, 2024 at 3:56 PM Jun Rao wrote: > > > > > Hi, Justine, > > > > > > Thanks for the updated KIP. A couple of more comments. > > > > > > 20. Could we show the output of version-mapping? > > > > > > 21. "Transaction version 1 will include the flexible fields in the > > > transaction state log, and transaction version 2 will include the > changes > > > to the transactional protocol as described by KIP-890 (epoch bumps and > > > implicit add partitions.)" > > > So TV 1 enables the writing of new tagged fields like PrevProducerId? > > But > > > those fields are only usable after the epoch bump, right? What > > > functionality does TV 1 achieve? > > > > > > Jun > > > > > > > > > On Mon, Apr 1, 2024 at 2:06 PM Justine Olshan > > > > > > > > wrote: > > > > > > > I have also updated the KIP to mention the feature tool's --metadata > > flag > > > > will be deprecated. > > > > It will still work for users as they learn the new flag, but a > warning > > > > indicating the alternatives will be shown. > > > > > > > > Justine > > > > > > > > On Thu, Mar 28, 2024 at 11:03 AM Justine Olshan < > jols...@confluent.io> > > > > wrote: > > > > > > > > > Hi Jun, > > > > > > > > > > For both transaction state and group coordinator state, there are > > only > > > > > version 0 records. > > > > > KIP-915 introduced flexible versions, but it was never put to use. > MV > > > has > > > > > never gated these. This KIP will do that. I can include this > context > > in > > > > the > > > > > KIP. > > > > > > > > > > I'm happy to modify his 1 and 2 to 0 and 1. > > > > > > > > > > Justine > > > > > > > > > > On Thu, Mar 28, 2024 at 10:57 AM Jun Rao > > > > > wrote: > > > > > > > > > >> Hi, David, > > > > >> > > > > >> Thanks for the reply. > > > > >> > > > > >> Historically, the format of all records were controlled by MV. > Now, > > > > >> records > > > > >> in _offset_commit will be controlled by > `group.coordinator.version`, > > > is > > > > >> that right? It would be useful to document that. > > > > >> > > > > >> Also, we should align on the version numbering. "kafka-feature > > > disable" > > > > >> says "Disable one or more feature flags. This is the same as > > > downgrading > > > > >> the version to zero". So, in the `group.coordinator.version' case, > > we > > > > >> probably should use version 0 for the old consumer protocol. > > > > >> > > > > >> Jun > > > > >> > > > > >> On Thu, Mar 28, 2024 at 2:13 AM Andrew Schofield < > > > > >> andrew_schofield_j...@outlook.com> wrote: > > > > >> > > > > >> > Hi David, > > > > >> > I agree that we should use the same mechanism to gate KIP-932 > once > > > > that > > > > >> > feature reaches production readiness. The precise details of the > > > > values > > > > >> > will > > > > >> > depend upon the current state of all these flags when that > release > > > > >> comes. > > > > >> > > > > > >> > Thanks, > > > > >> > Andrew > > > > >> > > > > > >> > > On 28 Mar 2024, at 07:11, David Jacot > > > > > > > > > >> > wrote: > > > > >> > > > > > > >> > > Hi, Jun, Justine, > > > > >> > > > > > > >> > > Regarding `group.coordinator.version`, the idea is to use it > to > > > gate > > > > >> > > records and APIs of the group coordinator. The first use case > > will > > > > be > > > > >> > > KIP-848. We will use version 2 of the flag to gate all the new > > > > records > > > > >> > and > > > > >> > > the new ConsumerGroupHeartbeat/Describe APIs present in AK > 3.8. > > So > > > > >> > version > > > > >> > > 1 will be the only the old protocol and version 2 will be the > > > > >> currently > > > > >> > > implemented new protocol. I don't think that we have any > > > dependency > > > > on > > > > >> > the > > > > >> > > metadata version at the moment. The changes are orthogonal. I > > > think > > > > >> that > > > > >> > we > > > > >> > > could mention KIP-848 as the first usage of this flag in the > > KIP. > > > I > > > > >> will > > > > >> > > also update KIP-848 to include
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi, Justine, Thanks for the reply. 21. What about the new NextProducerId field? Will that be populated with TV 1? 22. In the dependencies output, should we show both IV and level for metadata.version too? Jun On Mon, Apr 1, 2024 at 4:43 PM Justine Olshan wrote: > Hi Jun, > > 20. I can update the KIP. > > 21. This is used to complete some of the work with KIP-360. (We use > previous producer ID there, but never persisted it which was in the KIP > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=89068820) > The KIP also mentions including previous epoch but we explained in this KIP > how we can figure this out. > > Justine > > > > On Mon, Apr 1, 2024 at 3:56 PM Jun Rao wrote: > > > Hi, Justine, > > > > Thanks for the updated KIP. A couple of more comments. > > > > 20. Could we show the output of version-mapping? > > > > 21. "Transaction version 1 will include the flexible fields in the > > transaction state log, and transaction version 2 will include the changes > > to the transactional protocol as described by KIP-890 (epoch bumps and > > implicit add partitions.)" > > So TV 1 enables the writing of new tagged fields like PrevProducerId? > But > > those fields are only usable after the epoch bump, right? What > > functionality does TV 1 achieve? > > > > Jun > > > > > > On Mon, Apr 1, 2024 at 2:06 PM Justine Olshan > > > > > wrote: > > > > > I have also updated the KIP to mention the feature tool's --metadata > flag > > > will be deprecated. > > > It will still work for users as they learn the new flag, but a warning > > > indicating the alternatives will be shown. > > > > > > Justine > > > > > > On Thu, Mar 28, 2024 at 11:03 AM Justine Olshan > > > wrote: > > > > > > > Hi Jun, > > > > > > > > For both transaction state and group coordinator state, there are > only > > > > version 0 records. > > > > KIP-915 introduced flexible versions, but it was never put to use. MV > > has > > > > never gated these. This KIP will do that. I can include this context > in > > > the > > > > KIP. > > > > > > > > I'm happy to modify his 1 and 2 to 0 and 1. > > > > > > > > Justine > > > > > > > > On Thu, Mar 28, 2024 at 10:57 AM Jun Rao > > > wrote: > > > > > > > >> Hi, David, > > > >> > > > >> Thanks for the reply. > > > >> > > > >> Historically, the format of all records were controlled by MV. Now, > > > >> records > > > >> in _offset_commit will be controlled by `group.coordinator.version`, > > is > > > >> that right? It would be useful to document that. > > > >> > > > >> Also, we should align on the version numbering. "kafka-feature > > disable" > > > >> says "Disable one or more feature flags. This is the same as > > downgrading > > > >> the version to zero". So, in the `group.coordinator.version' case, > we > > > >> probably should use version 0 for the old consumer protocol. > > > >> > > > >> Jun > > > >> > > > >> On Thu, Mar 28, 2024 at 2:13 AM Andrew Schofield < > > > >> andrew_schofield_j...@outlook.com> wrote: > > > >> > > > >> > Hi David, > > > >> > I agree that we should use the same mechanism to gate KIP-932 once > > > that > > > >> > feature reaches production readiness. The precise details of the > > > values > > > >> > will > > > >> > depend upon the current state of all these flags when that release > > > >> comes. > > > >> > > > > >> > Thanks, > > > >> > Andrew > > > >> > > > > >> > > On 28 Mar 2024, at 07:11, David Jacot > > > > > > >> > wrote: > > > >> > > > > > >> > > Hi, Jun, Justine, > > > >> > > > > > >> > > Regarding `group.coordinator.version`, the idea is to use it to > > gate > > > >> > > records and APIs of the group coordinator. The first use case > will > > > be > > > >> > > KIP-848. We will use version 2 of the flag to gate all the new > > > records > > > >> > and > > > >> > > the new ConsumerGroupHeartbeat/Describe APIs present in AK 3.8. > So > > > >> > version > > > >> > > 1 will be the only the old protocol and version 2 will be the > > > >> currently > > > >> > > implemented new protocol. I don't think that we have any > > dependency > > > on > > > >> > the > > > >> > > metadata version at the moment. The changes are orthogonal. I > > think > > > >> that > > > >> > we > > > >> > > could mention KIP-848 as the first usage of this flag in the > KIP. > > I > > > >> will > > > >> > > also update KIP-848 to include it when this KIP is accepted. > > Another > > > >> use > > > >> > > case is the Queues KIP. I think that we should also use this new > > > flag > > > >> to > > > >> > > gate it. > > > >> > > > > > >> > > Best, > > > >> > > David > > > >> > > > > > >> > > On Thu, Mar 28, 2024 at 1:14 AM Jun Rao > > > > > > >> > wrote: > > > >> > > > > > >> > >> Hi, Justine, > > > >> > >> > > > >> > >> Thanks for the reply. > > > >> > >> > > > >> > >> So, "dependencies" and "version-mapping" will be added to both > > > >> > >> kafka-feature and kafka-storage? Could we document that in the > > tool > > > >> > format > > > >> > >> section? > > > >> > >> > > > >> > >> Jun >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi Justine, Thanks for the KIP. This will be very helpful! I do have one question regarding the naming of the new flags which is not totally clear in the KIP. It would be great if we could call them out in the Public Interfaces section. My understanding is that the KIP proposes to use `transaction.protocol.version` and `group.coordinator.version`. I was wondering whether we should just use `transaction.version` and `group.version`. The rationale for the first one is that a new version may not always be for a protocol change. The rationale for the second one is that it gates more than the group coordinator as we may use it for queues too. It would also be aligned with `metadata.version`. I apologize if this was already discussed. Best, David On Tue, Apr 2, 2024 at 11:18 AM David Jacot wrote: > Hi Jun, > > > Historically, the format of all records were controlled by MV. Now, > records > in _offset_commit will be controlled by `group.coordinator.version`, is > that right? It would be useful to document that. > > Yes. This is correct. The idea is to replace the MV with this new flag. It > will have the same semantics but with the benefit of being independent. > > > Also, we should align on the version numbering. "kafka-feature disable" > says "Disable one or more feature flags. This is the same as downgrading > the version to zero". So, in the `group.coordinator.version' case, we > probably should use version 0 for the old consumer protocol. > > This makes sense. We can definitely use version 0. > > Best, > David > > On Tue, Apr 2, 2024 at 1:43 AM Justine Olshan > wrote: > >> Hi Jun, >> >> 20. I can update the KIP. >> >> 21. This is used to complete some of the work with KIP-360. (We use >> previous producer ID there, but never persisted it which was in the KIP >> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=89068820 >> ) >> The KIP also mentions including previous epoch but we explained in this >> KIP >> how we can figure this out. >> >> Justine >> >> >> >> On Mon, Apr 1, 2024 at 3:56 PM Jun Rao wrote: >> >> > Hi, Justine, >> > >> > Thanks for the updated KIP. A couple of more comments. >> > >> > 20. Could we show the output of version-mapping? >> > >> > 21. "Transaction version 1 will include the flexible fields in the >> > transaction state log, and transaction version 2 will include the >> changes >> > to the transactional protocol as described by KIP-890 (epoch bumps and >> > implicit add partitions.)" >> > So TV 1 enables the writing of new tagged fields like PrevProducerId? >> But >> > those fields are only usable after the epoch bump, right? What >> > functionality does TV 1 achieve? >> > >> > Jun >> > >> > >> > On Mon, Apr 1, 2024 at 2:06 PM Justine Olshan >> > > > >> > wrote: >> > >> > > I have also updated the KIP to mention the feature tool's --metadata >> flag >> > > will be deprecated. >> > > It will still work for users as they learn the new flag, but a warning >> > > indicating the alternatives will be shown. >> > > >> > > Justine >> > > >> > > On Thu, Mar 28, 2024 at 11:03 AM Justine Olshan > > >> > > wrote: >> > > >> > > > Hi Jun, >> > > > >> > > > For both transaction state and group coordinator state, there are >> only >> > > > version 0 records. >> > > > KIP-915 introduced flexible versions, but it was never put to use. >> MV >> > has >> > > > never gated these. This KIP will do that. I can include this >> context in >> > > the >> > > > KIP. >> > > > >> > > > I'm happy to modify his 1 and 2 to 0 and 1. >> > > > >> > > > Justine >> > > > >> > > > On Thu, Mar 28, 2024 at 10:57 AM Jun Rao >> > > wrote: >> > > > >> > > >> Hi, David, >> > > >> >> > > >> Thanks for the reply. >> > > >> >> > > >> Historically, the format of all records were controlled by MV. Now, >> > > >> records >> > > >> in _offset_commit will be controlled by >> `group.coordinator.version`, >> > is >> > > >> that right? It would be useful to document that. >> > > >> >> > > >> Also, we should align on the version numbering. "kafka-feature >> > disable" >> > > >> says "Disable one or more feature flags. This is the same as >> > downgrading >> > > >> the version to zero". So, in the `group.coordinator.version' case, >> we >> > > >> probably should use version 0 for the old consumer protocol. >> > > >> >> > > >> Jun >> > > >> >> > > >> On Thu, Mar 28, 2024 at 2:13 AM Andrew Schofield < >> > > >> andrew_schofield_j...@outlook.com> wrote: >> > > >> >> > > >> > Hi David, >> > > >> > I agree that we should use the same mechanism to gate KIP-932 >> once >> > > that >> > > >> > feature reaches production readiness. The precise details of the >> > > values >> > > >> > will >> > > >> > depend upon the current state of all these flags when that >> release >> > > >> comes. >> > > >> > >> > > >> > Thanks, >> > > >> > Andrew >> > > >> > >> > > >> > > On 28 Mar 2024, at 07:11, David Jacot >> > > > >> > > >> > wrote: >> > > >> > > >> > > >> > > Hi, Jun, Justine, >> > > >> > > >> > > >> > > Regarding
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi Jun, > Historically, the format of all records were controlled by MV. Now, records in _offset_commit will be controlled by `group.coordinator.version`, is that right? It would be useful to document that. Yes. This is correct. The idea is to replace the MV with this new flag. It will have the same semantics but with the benefit of being independent. > Also, we should align on the version numbering. "kafka-feature disable" says "Disable one or more feature flags. This is the same as downgrading the version to zero". So, in the `group.coordinator.version' case, we probably should use version 0 for the old consumer protocol. This makes sense. We can definitely use version 0. Best, David On Tue, Apr 2, 2024 at 1:43 AM Justine Olshan wrote: > Hi Jun, > > 20. I can update the KIP. > > 21. This is used to complete some of the work with KIP-360. (We use > previous producer ID there, but never persisted it which was in the KIP > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=89068820) > The KIP also mentions including previous epoch but we explained in this KIP > how we can figure this out. > > Justine > > > > On Mon, Apr 1, 2024 at 3:56 PM Jun Rao wrote: > > > Hi, Justine, > > > > Thanks for the updated KIP. A couple of more comments. > > > > 20. Could we show the output of version-mapping? > > > > 21. "Transaction version 1 will include the flexible fields in the > > transaction state log, and transaction version 2 will include the changes > > to the transactional protocol as described by KIP-890 (epoch bumps and > > implicit add partitions.)" > > So TV 1 enables the writing of new tagged fields like PrevProducerId? > But > > those fields are only usable after the epoch bump, right? What > > functionality does TV 1 achieve? > > > > Jun > > > > > > On Mon, Apr 1, 2024 at 2:06 PM Justine Olshan > > > > > wrote: > > > > > I have also updated the KIP to mention the feature tool's --metadata > flag > > > will be deprecated. > > > It will still work for users as they learn the new flag, but a warning > > > indicating the alternatives will be shown. > > > > > > Justine > > > > > > On Thu, Mar 28, 2024 at 11:03 AM Justine Olshan > > > wrote: > > > > > > > Hi Jun, > > > > > > > > For both transaction state and group coordinator state, there are > only > > > > version 0 records. > > > > KIP-915 introduced flexible versions, but it was never put to use. MV > > has > > > > never gated these. This KIP will do that. I can include this context > in > > > the > > > > KIP. > > > > > > > > I'm happy to modify his 1 and 2 to 0 and 1. > > > > > > > > Justine > > > > > > > > On Thu, Mar 28, 2024 at 10:57 AM Jun Rao > > > wrote: > > > > > > > >> Hi, David, > > > >> > > > >> Thanks for the reply. > > > >> > > > >> Historically, the format of all records were controlled by MV. Now, > > > >> records > > > >> in _offset_commit will be controlled by `group.coordinator.version`, > > is > > > >> that right? It would be useful to document that. > > > >> > > > >> Also, we should align on the version numbering. "kafka-feature > > disable" > > > >> says "Disable one or more feature flags. This is the same as > > downgrading > > > >> the version to zero". So, in the `group.coordinator.version' case, > we > > > >> probably should use version 0 for the old consumer protocol. > > > >> > > > >> Jun > > > >> > > > >> On Thu, Mar 28, 2024 at 2:13 AM Andrew Schofield < > > > >> andrew_schofield_j...@outlook.com> wrote: > > > >> > > > >> > Hi David, > > > >> > I agree that we should use the same mechanism to gate KIP-932 once > > > that > > > >> > feature reaches production readiness. The precise details of the > > > values > > > >> > will > > > >> > depend upon the current state of all these flags when that release > > > >> comes. > > > >> > > > > >> > Thanks, > > > >> > Andrew > > > >> > > > > >> > > On 28 Mar 2024, at 07:11, David Jacot > > > > > > >> > wrote: > > > >> > > > > > >> > > Hi, Jun, Justine, > > > >> > > > > > >> > > Regarding `group.coordinator.version`, the idea is to use it to > > gate > > > >> > > records and APIs of the group coordinator. The first use case > will > > > be > > > >> > > KIP-848. We will use version 2 of the flag to gate all the new > > > records > > > >> > and > > > >> > > the new ConsumerGroupHeartbeat/Describe APIs present in AK 3.8. > So > > > >> > version > > > >> > > 1 will be the only the old protocol and version 2 will be the > > > >> currently > > > >> > > implemented new protocol. I don't think that we have any > > dependency > > > on > > > >> > the > > > >> > > metadata version at the moment. The changes are orthogonal. I > > think > > > >> that > > > >> > we > > > >> > > could mention KIP-848 as the first usage of this flag in the > KIP. > > I > > > >> will > > > >> > > also update KIP-848 to include it when this KIP is accepted. > > Another > > > >> use > > > >> > > case is the Queues KIP. I think that we should also use this new > > > flag > > > >> to > > > >> > > gate
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi Jun, 20. I can update the KIP. 21. This is used to complete some of the work with KIP-360. (We use previous producer ID there, but never persisted it which was in the KIP https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=89068820) The KIP also mentions including previous epoch but we explained in this KIP how we can figure this out. Justine On Mon, Apr 1, 2024 at 3:56 PM Jun Rao wrote: > Hi, Justine, > > Thanks for the updated KIP. A couple of more comments. > > 20. Could we show the output of version-mapping? > > 21. "Transaction version 1 will include the flexible fields in the > transaction state log, and transaction version 2 will include the changes > to the transactional protocol as described by KIP-890 (epoch bumps and > implicit add partitions.)" > So TV 1 enables the writing of new tagged fields like PrevProducerId? But > those fields are only usable after the epoch bump, right? What > functionality does TV 1 achieve? > > Jun > > > On Mon, Apr 1, 2024 at 2:06 PM Justine Olshan > > wrote: > > > I have also updated the KIP to mention the feature tool's --metadata flag > > will be deprecated. > > It will still work for users as they learn the new flag, but a warning > > indicating the alternatives will be shown. > > > > Justine > > > > On Thu, Mar 28, 2024 at 11:03 AM Justine Olshan > > wrote: > > > > > Hi Jun, > > > > > > For both transaction state and group coordinator state, there are only > > > version 0 records. > > > KIP-915 introduced flexible versions, but it was never put to use. MV > has > > > never gated these. This KIP will do that. I can include this context in > > the > > > KIP. > > > > > > I'm happy to modify his 1 and 2 to 0 and 1. > > > > > > Justine > > > > > > On Thu, Mar 28, 2024 at 10:57 AM Jun Rao > > wrote: > > > > > >> Hi, David, > > >> > > >> Thanks for the reply. > > >> > > >> Historically, the format of all records were controlled by MV. Now, > > >> records > > >> in _offset_commit will be controlled by `group.coordinator.version`, > is > > >> that right? It would be useful to document that. > > >> > > >> Also, we should align on the version numbering. "kafka-feature > disable" > > >> says "Disable one or more feature flags. This is the same as > downgrading > > >> the version to zero". So, in the `group.coordinator.version' case, we > > >> probably should use version 0 for the old consumer protocol. > > >> > > >> Jun > > >> > > >> On Thu, Mar 28, 2024 at 2:13 AM Andrew Schofield < > > >> andrew_schofield_j...@outlook.com> wrote: > > >> > > >> > Hi David, > > >> > I agree that we should use the same mechanism to gate KIP-932 once > > that > > >> > feature reaches production readiness. The precise details of the > > values > > >> > will > > >> > depend upon the current state of all these flags when that release > > >> comes. > > >> > > > >> > Thanks, > > >> > Andrew > > >> > > > >> > > On 28 Mar 2024, at 07:11, David Jacot > > > >> > wrote: > > >> > > > > >> > > Hi, Jun, Justine, > > >> > > > > >> > > Regarding `group.coordinator.version`, the idea is to use it to > gate > > >> > > records and APIs of the group coordinator. The first use case will > > be > > >> > > KIP-848. We will use version 2 of the flag to gate all the new > > records > > >> > and > > >> > > the new ConsumerGroupHeartbeat/Describe APIs present in AK 3.8. So > > >> > version > > >> > > 1 will be the only the old protocol and version 2 will be the > > >> currently > > >> > > implemented new protocol. I don't think that we have any > dependency > > on > > >> > the > > >> > > metadata version at the moment. The changes are orthogonal. I > think > > >> that > > >> > we > > >> > > could mention KIP-848 as the first usage of this flag in the KIP. > I > > >> will > > >> > > also update KIP-848 to include it when this KIP is accepted. > Another > > >> use > > >> > > case is the Queues KIP. I think that we should also use this new > > flag > > >> to > > >> > > gate it. > > >> > > > > >> > > Best, > > >> > > David > > >> > > > > >> > > On Thu, Mar 28, 2024 at 1:14 AM Jun Rao > > > >> > wrote: > > >> > > > > >> > >> Hi, Justine, > > >> > >> > > >> > >> Thanks for the reply. > > >> > >> > > >> > >> So, "dependencies" and "version-mapping" will be added to both > > >> > >> kafka-feature and kafka-storage? Could we document that in the > tool > > >> > format > > >> > >> section? > > >> > >> > > >> > >> Jun > > >> > >> > > >> > >> On Wed, Mar 27, 2024 at 4:01 PM Justine Olshan > > >> > >> > > >> > >> wrote: > > >> > >> > > >> > >>> Ok. I can remove the info from the describe output. > > >> > >>> > > >> > >>> Dependencies is needed for the storage tool because we want to > > make > > >> > sure > > >> > >>> the desired versions we are setting will be valid. Version > mapping > > >> > should > > >> > >>> be for both tools since we have --release-version for both > tools. > > >> > >>> > > >> > >>> I was considering changing the IV strings, but I wasn't sure if > > >> there > > >> > >>
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi, Justine, Thanks for the updated KIP. A couple of more comments. 20. Could we show the output of version-mapping? 21. "Transaction version 1 will include the flexible fields in the transaction state log, and transaction version 2 will include the changes to the transactional protocol as described by KIP-890 (epoch bumps and implicit add partitions.)" So TV 1 enables the writing of new tagged fields like PrevProducerId? But those fields are only usable after the epoch bump, right? What functionality does TV 1 achieve? Jun On Mon, Apr 1, 2024 at 2:06 PM Justine Olshan wrote: > I have also updated the KIP to mention the feature tool's --metadata flag > will be deprecated. > It will still work for users as they learn the new flag, but a warning > indicating the alternatives will be shown. > > Justine > > On Thu, Mar 28, 2024 at 11:03 AM Justine Olshan > wrote: > > > Hi Jun, > > > > For both transaction state and group coordinator state, there are only > > version 0 records. > > KIP-915 introduced flexible versions, but it was never put to use. MV has > > never gated these. This KIP will do that. I can include this context in > the > > KIP. > > > > I'm happy to modify his 1 and 2 to 0 and 1. > > > > Justine > > > > On Thu, Mar 28, 2024 at 10:57 AM Jun Rao > wrote: > > > >> Hi, David, > >> > >> Thanks for the reply. > >> > >> Historically, the format of all records were controlled by MV. Now, > >> records > >> in _offset_commit will be controlled by `group.coordinator.version`, is > >> that right? It would be useful to document that. > >> > >> Also, we should align on the version numbering. "kafka-feature disable" > >> says "Disable one or more feature flags. This is the same as downgrading > >> the version to zero". So, in the `group.coordinator.version' case, we > >> probably should use version 0 for the old consumer protocol. > >> > >> Jun > >> > >> On Thu, Mar 28, 2024 at 2:13 AM Andrew Schofield < > >> andrew_schofield_j...@outlook.com> wrote: > >> > >> > Hi David, > >> > I agree that we should use the same mechanism to gate KIP-932 once > that > >> > feature reaches production readiness. The precise details of the > values > >> > will > >> > depend upon the current state of all these flags when that release > >> comes. > >> > > >> > Thanks, > >> > Andrew > >> > > >> > > On 28 Mar 2024, at 07:11, David Jacot > >> > wrote: > >> > > > >> > > Hi, Jun, Justine, > >> > > > >> > > Regarding `group.coordinator.version`, the idea is to use it to gate > >> > > records and APIs of the group coordinator. The first use case will > be > >> > > KIP-848. We will use version 2 of the flag to gate all the new > records > >> > and > >> > > the new ConsumerGroupHeartbeat/Describe APIs present in AK 3.8. So > >> > version > >> > > 1 will be the only the old protocol and version 2 will be the > >> currently > >> > > implemented new protocol. I don't think that we have any dependency > on > >> > the > >> > > metadata version at the moment. The changes are orthogonal. I think > >> that > >> > we > >> > > could mention KIP-848 as the first usage of this flag in the KIP. I > >> will > >> > > also update KIP-848 to include it when this KIP is accepted. Another > >> use > >> > > case is the Queues KIP. I think that we should also use this new > flag > >> to > >> > > gate it. > >> > > > >> > > Best, > >> > > David > >> > > > >> > > On Thu, Mar 28, 2024 at 1:14 AM Jun Rao > >> > wrote: > >> > > > >> > >> Hi, Justine, > >> > >> > >> > >> Thanks for the reply. > >> > >> > >> > >> So, "dependencies" and "version-mapping" will be added to both > >> > >> kafka-feature and kafka-storage? Could we document that in the tool > >> > format > >> > >> section? > >> > >> > >> > >> Jun > >> > >> > >> > >> On Wed, Mar 27, 2024 at 4:01 PM Justine Olshan > >> > >> > >> > >> wrote: > >> > >> > >> > >>> Ok. I can remove the info from the describe output. > >> > >>> > >> > >>> Dependencies is needed for the storage tool because we want to > make > >> > sure > >> > >>> the desired versions we are setting will be valid. Version mapping > >> > should > >> > >>> be for both tools since we have --release-version for both tools. > >> > >>> > >> > >>> I was considering changing the IV strings, but I wasn't sure if > >> there > >> > >> would > >> > >>> be some disagreement with the decision. Not sure if that breaks > >> > >>> compatibility etc. Happy to hear everyone's thoughts. > >> > >>> > >> > >>> Justine > >> > >>> > >> > >>> On Wed, Mar 27, 2024 at 3:36 PM Jun Rao > > >> > >> wrote: > >> > >>> > >> > Hi, Justine, > >> > > >> > Thanks for the reply. > >> > > >> > Having "kafka-feature dependencies" seems enough to me. We don't > >> need > >> > >> to > >> > include the dependencies in the output of "kafka-feature > describe". > >> > > >> > We only support "dependencies" in kafka-feature, not > >> kafka-storage. We > >> > probably should do the same for "version-mapping". > >> > > >> >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
I have also updated the KIP to mention the feature tool's --metadata flag will be deprecated. It will still work for users as they learn the new flag, but a warning indicating the alternatives will be shown. Justine On Thu, Mar 28, 2024 at 11:03 AM Justine Olshan wrote: > Hi Jun, > > For both transaction state and group coordinator state, there are only > version 0 records. > KIP-915 introduced flexible versions, but it was never put to use. MV has > never gated these. This KIP will do that. I can include this context in the > KIP. > > I'm happy to modify his 1 and 2 to 0 and 1. > > Justine > > On Thu, Mar 28, 2024 at 10:57 AM Jun Rao wrote: > >> Hi, David, >> >> Thanks for the reply. >> >> Historically, the format of all records were controlled by MV. Now, >> records >> in _offset_commit will be controlled by `group.coordinator.version`, is >> that right? It would be useful to document that. >> >> Also, we should align on the version numbering. "kafka-feature disable" >> says "Disable one or more feature flags. This is the same as downgrading >> the version to zero". So, in the `group.coordinator.version' case, we >> probably should use version 0 for the old consumer protocol. >> >> Jun >> >> On Thu, Mar 28, 2024 at 2:13 AM Andrew Schofield < >> andrew_schofield_j...@outlook.com> wrote: >> >> > Hi David, >> > I agree that we should use the same mechanism to gate KIP-932 once that >> > feature reaches production readiness. The precise details of the values >> > will >> > depend upon the current state of all these flags when that release >> comes. >> > >> > Thanks, >> > Andrew >> > >> > > On 28 Mar 2024, at 07:11, David Jacot >> > wrote: >> > > >> > > Hi, Jun, Justine, >> > > >> > > Regarding `group.coordinator.version`, the idea is to use it to gate >> > > records and APIs of the group coordinator. The first use case will be >> > > KIP-848. We will use version 2 of the flag to gate all the new records >> > and >> > > the new ConsumerGroupHeartbeat/Describe APIs present in AK 3.8. So >> > version >> > > 1 will be the only the old protocol and version 2 will be the >> currently >> > > implemented new protocol. I don't think that we have any dependency on >> > the >> > > metadata version at the moment. The changes are orthogonal. I think >> that >> > we >> > > could mention KIP-848 as the first usage of this flag in the KIP. I >> will >> > > also update KIP-848 to include it when this KIP is accepted. Another >> use >> > > case is the Queues KIP. I think that we should also use this new flag >> to >> > > gate it. >> > > >> > > Best, >> > > David >> > > >> > > On Thu, Mar 28, 2024 at 1:14 AM Jun Rao >> > wrote: >> > > >> > >> Hi, Justine, >> > >> >> > >> Thanks for the reply. >> > >> >> > >> So, "dependencies" and "version-mapping" will be added to both >> > >> kafka-feature and kafka-storage? Could we document that in the tool >> > format >> > >> section? >> > >> >> > >> Jun >> > >> >> > >> On Wed, Mar 27, 2024 at 4:01 PM Justine Olshan >> > >> >> > >> wrote: >> > >> >> > >>> Ok. I can remove the info from the describe output. >> > >>> >> > >>> Dependencies is needed for the storage tool because we want to make >> > sure >> > >>> the desired versions we are setting will be valid. Version mapping >> > should >> > >>> be for both tools since we have --release-version for both tools. >> > >>> >> > >>> I was considering changing the IV strings, but I wasn't sure if >> there >> > >> would >> > >>> be some disagreement with the decision. Not sure if that breaks >> > >>> compatibility etc. Happy to hear everyone's thoughts. >> > >>> >> > >>> Justine >> > >>> >> > >>> On Wed, Mar 27, 2024 at 3:36 PM Jun Rao >> > >> wrote: >> > >>> >> > Hi, Justine, >> > >> > Thanks for the reply. >> > >> > Having "kafka-feature dependencies" seems enough to me. We don't >> need >> > >> to >> > include the dependencies in the output of "kafka-feature describe". >> > >> > We only support "dependencies" in kafka-feature, not >> kafka-storage. We >> > probably should do the same for "version-mapping". >> > >> > bin/kafka-features.sh downgrade --feature metadata.version=16 >> > --transaction.protocol.version=2 >> > We need to add the --feature flag for the second feature, right? >> > >> > In "kafka-features.sh describe", we only show the IV string for >> > metadata.version. Should we also show the level number? >> > >> > Thanks, >> > >> > Jun >> > >> > On Wed, Mar 27, 2024 at 1:52 PM Justine Olshan >> > >> > wrote: >> > >> > > I had already included this example >> > > bin/kafka-features.sh downgrade --feature metadata.version=16 >> > > --transaction.protocol.version=2 // Throws error if metadata >> version >> > >>> is < >> > > 16, and this would be an upgrade >> > > But I have updated the KIP to explicitly say the text you >> mentioned. >> > > >> > > Justine >> > > >> >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi Jun, For both transaction state and group coordinator state, there are only version 0 records. KIP-915 introduced flexible versions, but it was never put to use. MV has never gated these. This KIP will do that. I can include this context in the KIP. I'm happy to modify his 1 and 2 to 0 and 1. Justine On Thu, Mar 28, 2024 at 10:57 AM Jun Rao wrote: > Hi, David, > > Thanks for the reply. > > Historically, the format of all records were controlled by MV. Now, records > in _offset_commit will be controlled by `group.coordinator.version`, is > that right? It would be useful to document that. > > Also, we should align on the version numbering. "kafka-feature disable" > says "Disable one or more feature flags. This is the same as downgrading > the version to zero". So, in the `group.coordinator.version' case, we > probably should use version 0 for the old consumer protocol. > > Jun > > On Thu, Mar 28, 2024 at 2:13 AM Andrew Schofield < > andrew_schofield_j...@outlook.com> wrote: > > > Hi David, > > I agree that we should use the same mechanism to gate KIP-932 once that > > feature reaches production readiness. The precise details of the values > > will > > depend upon the current state of all these flags when that release comes. > > > > Thanks, > > Andrew > > > > > On 28 Mar 2024, at 07:11, David Jacot > > wrote: > > > > > > Hi, Jun, Justine, > > > > > > Regarding `group.coordinator.version`, the idea is to use it to gate > > > records and APIs of the group coordinator. The first use case will be > > > KIP-848. We will use version 2 of the flag to gate all the new records > > and > > > the new ConsumerGroupHeartbeat/Describe APIs present in AK 3.8. So > > version > > > 1 will be the only the old protocol and version 2 will be the currently > > > implemented new protocol. I don't think that we have any dependency on > > the > > > metadata version at the moment. The changes are orthogonal. I think > that > > we > > > could mention KIP-848 as the first usage of this flag in the KIP. I > will > > > also update KIP-848 to include it when this KIP is accepted. Another > use > > > case is the Queues KIP. I think that we should also use this new flag > to > > > gate it. > > > > > > Best, > > > David > > > > > > On Thu, Mar 28, 2024 at 1:14 AM Jun Rao > > wrote: > > > > > >> Hi, Justine, > > >> > > >> Thanks for the reply. > > >> > > >> So, "dependencies" and "version-mapping" will be added to both > > >> kafka-feature and kafka-storage? Could we document that in the tool > > format > > >> section? > > >> > > >> Jun > > >> > > >> On Wed, Mar 27, 2024 at 4:01 PM Justine Olshan > > >> > > >> wrote: > > >> > > >>> Ok. I can remove the info from the describe output. > > >>> > > >>> Dependencies is needed for the storage tool because we want to make > > sure > > >>> the desired versions we are setting will be valid. Version mapping > > should > > >>> be for both tools since we have --release-version for both tools. > > >>> > > >>> I was considering changing the IV strings, but I wasn't sure if there > > >> would > > >>> be some disagreement with the decision. Not sure if that breaks > > >>> compatibility etc. Happy to hear everyone's thoughts. > > >>> > > >>> Justine > > >>> > > >>> On Wed, Mar 27, 2024 at 3:36 PM Jun Rao > > >> wrote: > > >>> > > Hi, Justine, > > > > Thanks for the reply. > > > > Having "kafka-feature dependencies" seems enough to me. We don't > need > > >> to > > include the dependencies in the output of "kafka-feature describe". > > > > We only support "dependencies" in kafka-feature, not kafka-storage. > We > > probably should do the same for "version-mapping". > > > > bin/kafka-features.sh downgrade --feature metadata.version=16 > > --transaction.protocol.version=2 > > We need to add the --feature flag for the second feature, right? > > > > In "kafka-features.sh describe", we only show the IV string for > > metadata.version. Should we also show the level number? > > > > Thanks, > > > > Jun > > > > On Wed, Mar 27, 2024 at 1:52 PM Justine Olshan > > > > wrote: > > > > > I had already included this example > > > bin/kafka-features.sh downgrade --feature metadata.version=16 > > > --transaction.protocol.version=2 // Throws error if metadata > version > > >>> is < > > > 16, and this would be an upgrade > > > But I have updated the KIP to explicitly say the text you > mentioned. > > > > > > Justine > > > > > > On Wed, Mar 27, 2024 at 1:41 PM José Armando García Sancio > > > wrote: > > > > > >> Hi Justine, > > >> > > >> See my comment below. > > >> > > >> On Wed, Mar 27, 2024 at 1:31 PM Justine Olshan > > >> wrote: > > >>> The feature command includes the upgrade or downgrade command > > >> along > > > with > > >>> the --release-version flag. If some features are not moving in > > >> the > >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi, David, Thanks for the reply. Historically, the format of all records were controlled by MV. Now, records in _offset_commit will be controlled by `group.coordinator.version`, is that right? It would be useful to document that. Also, we should align on the version numbering. "kafka-feature disable" says "Disable one or more feature flags. This is the same as downgrading the version to zero". So, in the `group.coordinator.version' case, we probably should use version 0 for the old consumer protocol. Jun On Thu, Mar 28, 2024 at 2:13 AM Andrew Schofield < andrew_schofield_j...@outlook.com> wrote: > Hi David, > I agree that we should use the same mechanism to gate KIP-932 once that > feature reaches production readiness. The precise details of the values > will > depend upon the current state of all these flags when that release comes. > > Thanks, > Andrew > > > On 28 Mar 2024, at 07:11, David Jacot > wrote: > > > > Hi, Jun, Justine, > > > > Regarding `group.coordinator.version`, the idea is to use it to gate > > records and APIs of the group coordinator. The first use case will be > > KIP-848. We will use version 2 of the flag to gate all the new records > and > > the new ConsumerGroupHeartbeat/Describe APIs present in AK 3.8. So > version > > 1 will be the only the old protocol and version 2 will be the currently > > implemented new protocol. I don't think that we have any dependency on > the > > metadata version at the moment. The changes are orthogonal. I think that > we > > could mention KIP-848 as the first usage of this flag in the KIP. I will > > also update KIP-848 to include it when this KIP is accepted. Another use > > case is the Queues KIP. I think that we should also use this new flag to > > gate it. > > > > Best, > > David > > > > On Thu, Mar 28, 2024 at 1:14 AM Jun Rao > wrote: > > > >> Hi, Justine, > >> > >> Thanks for the reply. > >> > >> So, "dependencies" and "version-mapping" will be added to both > >> kafka-feature and kafka-storage? Could we document that in the tool > format > >> section? > >> > >> Jun > >> > >> On Wed, Mar 27, 2024 at 4:01 PM Justine Olshan > >> > >> wrote: > >> > >>> Ok. I can remove the info from the describe output. > >>> > >>> Dependencies is needed for the storage tool because we want to make > sure > >>> the desired versions we are setting will be valid. Version mapping > should > >>> be for both tools since we have --release-version for both tools. > >>> > >>> I was considering changing the IV strings, but I wasn't sure if there > >> would > >>> be some disagreement with the decision. Not sure if that breaks > >>> compatibility etc. Happy to hear everyone's thoughts. > >>> > >>> Justine > >>> > >>> On Wed, Mar 27, 2024 at 3:36 PM Jun Rao > >> wrote: > >>> > Hi, Justine, > > Thanks for the reply. > > Having "kafka-feature dependencies" seems enough to me. We don't need > >> to > include the dependencies in the output of "kafka-feature describe". > > We only support "dependencies" in kafka-feature, not kafka-storage. We > probably should do the same for "version-mapping". > > bin/kafka-features.sh downgrade --feature metadata.version=16 > --transaction.protocol.version=2 > We need to add the --feature flag for the second feature, right? > > In "kafka-features.sh describe", we only show the IV string for > metadata.version. Should we also show the level number? > > Thanks, > > Jun > > On Wed, Mar 27, 2024 at 1:52 PM Justine Olshan > > wrote: > > > I had already included this example > > bin/kafka-features.sh downgrade --feature metadata.version=16 > > --transaction.protocol.version=2 // Throws error if metadata version > >>> is < > > 16, and this would be an upgrade > > But I have updated the KIP to explicitly say the text you mentioned. > > > > Justine > > > > On Wed, Mar 27, 2024 at 1:41 PM José Armando García Sancio > > wrote: > > > >> Hi Justine, > >> > >> See my comment below. > >> > >> On Wed, Mar 27, 2024 at 1:31 PM Justine Olshan > >> wrote: > >>> The feature command includes the upgrade or downgrade command > >> along > > with > >>> the --release-version flag. If some features are not moving in > >> the > >>> direction mentioned (upgrade or downgrade) the command will fail > >> -- > >> perhaps > >>> with an error of which features were going in the wrong > >> direction. > >> > >> How about updating the KIP to show and document this behavior? > >> > >> Thanks, > >> -- > >> -José > >> > > > > >>> > >> > >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi David, I agree that we should use the same mechanism to gate KIP-932 once that feature reaches production readiness. The precise details of the values will depend upon the current state of all these flags when that release comes. Thanks, Andrew > On 28 Mar 2024, at 07:11, David Jacot wrote: > > Hi, Jun, Justine, > > Regarding `group.coordinator.version`, the idea is to use it to gate > records and APIs of the group coordinator. The first use case will be > KIP-848. We will use version 2 of the flag to gate all the new records and > the new ConsumerGroupHeartbeat/Describe APIs present in AK 3.8. So version > 1 will be the only the old protocol and version 2 will be the currently > implemented new protocol. I don't think that we have any dependency on the > metadata version at the moment. The changes are orthogonal. I think that we > could mention KIP-848 as the first usage of this flag in the KIP. I will > also update KIP-848 to include it when this KIP is accepted. Another use > case is the Queues KIP. I think that we should also use this new flag to > gate it. > > Best, > David > > On Thu, Mar 28, 2024 at 1:14 AM Jun Rao wrote: > >> Hi, Justine, >> >> Thanks for the reply. >> >> So, "dependencies" and "version-mapping" will be added to both >> kafka-feature and kafka-storage? Could we document that in the tool format >> section? >> >> Jun >> >> On Wed, Mar 27, 2024 at 4:01 PM Justine Olshan >> >> wrote: >> >>> Ok. I can remove the info from the describe output. >>> >>> Dependencies is needed for the storage tool because we want to make sure >>> the desired versions we are setting will be valid. Version mapping should >>> be for both tools since we have --release-version for both tools. >>> >>> I was considering changing the IV strings, but I wasn't sure if there >> would >>> be some disagreement with the decision. Not sure if that breaks >>> compatibility etc. Happy to hear everyone's thoughts. >>> >>> Justine >>> >>> On Wed, Mar 27, 2024 at 3:36 PM Jun Rao >> wrote: >>> Hi, Justine, Thanks for the reply. Having "kafka-feature dependencies" seems enough to me. We don't need >> to include the dependencies in the output of "kafka-feature describe". We only support "dependencies" in kafka-feature, not kafka-storage. We probably should do the same for "version-mapping". bin/kafka-features.sh downgrade --feature metadata.version=16 --transaction.protocol.version=2 We need to add the --feature flag for the second feature, right? In "kafka-features.sh describe", we only show the IV string for metadata.version. Should we also show the level number? Thanks, Jun On Wed, Mar 27, 2024 at 1:52 PM Justine Olshan wrote: > I had already included this example > bin/kafka-features.sh downgrade --feature metadata.version=16 > --transaction.protocol.version=2 // Throws error if metadata version >>> is < > 16, and this would be an upgrade > But I have updated the KIP to explicitly say the text you mentioned. > > Justine > > On Wed, Mar 27, 2024 at 1:41 PM José Armando García Sancio > wrote: > >> Hi Justine, >> >> See my comment below. >> >> On Wed, Mar 27, 2024 at 1:31 PM Justine Olshan >> wrote: >>> The feature command includes the upgrade or downgrade command >> along > with >>> the --release-version flag. If some features are not moving in >> the >>> direction mentioned (upgrade or downgrade) the command will fail >> -- >> perhaps >>> with an error of which features were going in the wrong >> direction. >> >> How about updating the KIP to show and document this behavior? >> >> Thanks, >> -- >> -José >> > >>> >>
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi, Jun, Justine, Regarding `group.coordinator.version`, the idea is to use it to gate records and APIs of the group coordinator. The first use case will be KIP-848. We will use version 2 of the flag to gate all the new records and the new ConsumerGroupHeartbeat/Describe APIs present in AK 3.8. So version 1 will be the only the old protocol and version 2 will be the currently implemented new protocol. I don't think that we have any dependency on the metadata version at the moment. The changes are orthogonal. I think that we could mention KIP-848 as the first usage of this flag in the KIP. I will also update KIP-848 to include it when this KIP is accepted. Another use case is the Queues KIP. I think that we should also use this new flag to gate it. Best, David On Thu, Mar 28, 2024 at 1:14 AM Jun Rao wrote: > Hi, Justine, > > Thanks for the reply. > > So, "dependencies" and "version-mapping" will be added to both > kafka-feature and kafka-storage? Could we document that in the tool format > section? > > Jun > > On Wed, Mar 27, 2024 at 4:01 PM Justine Olshan > > wrote: > > > Ok. I can remove the info from the describe output. > > > > Dependencies is needed for the storage tool because we want to make sure > > the desired versions we are setting will be valid. Version mapping should > > be for both tools since we have --release-version for both tools. > > > > I was considering changing the IV strings, but I wasn't sure if there > would > > be some disagreement with the decision. Not sure if that breaks > > compatibility etc. Happy to hear everyone's thoughts. > > > > Justine > > > > On Wed, Mar 27, 2024 at 3:36 PM Jun Rao > wrote: > > > > > Hi, Justine, > > > > > > Thanks for the reply. > > > > > > Having "kafka-feature dependencies" seems enough to me. We don't need > to > > > include the dependencies in the output of "kafka-feature describe". > > > > > > We only support "dependencies" in kafka-feature, not kafka-storage. We > > > probably should do the same for "version-mapping". > > > > > > bin/kafka-features.sh downgrade --feature metadata.version=16 > > > --transaction.protocol.version=2 > > > We need to add the --feature flag for the second feature, right? > > > > > > In "kafka-features.sh describe", we only show the IV string for > > > metadata.version. Should we also show the level number? > > > > > > Thanks, > > > > > > Jun > > > > > > On Wed, Mar 27, 2024 at 1:52 PM Justine Olshan > > > > > > wrote: > > > > > > > I had already included this example > > > > bin/kafka-features.sh downgrade --feature metadata.version=16 > > > > --transaction.protocol.version=2 // Throws error if metadata version > > is < > > > > 16, and this would be an upgrade > > > > But I have updated the KIP to explicitly say the text you mentioned. > > > > > > > > Justine > > > > > > > > On Wed, Mar 27, 2024 at 1:41 PM José Armando García Sancio > > > > wrote: > > > > > > > > > Hi Justine, > > > > > > > > > > See my comment below. > > > > > > > > > > On Wed, Mar 27, 2024 at 1:31 PM Justine Olshan > > > > > wrote: > > > > > > The feature command includes the upgrade or downgrade command > along > > > > with > > > > > > the --release-version flag. If some features are not moving in > the > > > > > > direction mentioned (upgrade or downgrade) the command will fail > -- > > > > > perhaps > > > > > > with an error of which features were going in the wrong > direction. > > > > > > > > > > How about updating the KIP to show and document this behavior? > > > > > > > > > > Thanks, > > > > > -- > > > > > -José > > > > > > > > > > > > > > >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi, Justine, Thanks for the reply. So, "dependencies" and "version-mapping" will be added to both kafka-feature and kafka-storage? Could we document that in the tool format section? Jun On Wed, Mar 27, 2024 at 4:01 PM Justine Olshan wrote: > Ok. I can remove the info from the describe output. > > Dependencies is needed for the storage tool because we want to make sure > the desired versions we are setting will be valid. Version mapping should > be for both tools since we have --release-version for both tools. > > I was considering changing the IV strings, but I wasn't sure if there would > be some disagreement with the decision. Not sure if that breaks > compatibility etc. Happy to hear everyone's thoughts. > > Justine > > On Wed, Mar 27, 2024 at 3:36 PM Jun Rao wrote: > > > Hi, Justine, > > > > Thanks for the reply. > > > > Having "kafka-feature dependencies" seems enough to me. We don't need to > > include the dependencies in the output of "kafka-feature describe". > > > > We only support "dependencies" in kafka-feature, not kafka-storage. We > > probably should do the same for "version-mapping". > > > > bin/kafka-features.sh downgrade --feature metadata.version=16 > > --transaction.protocol.version=2 > > We need to add the --feature flag for the second feature, right? > > > > In "kafka-features.sh describe", we only show the IV string for > > metadata.version. Should we also show the level number? > > > > Thanks, > > > > Jun > > > > On Wed, Mar 27, 2024 at 1:52 PM Justine Olshan > > > > wrote: > > > > > I had already included this example > > > bin/kafka-features.sh downgrade --feature metadata.version=16 > > > --transaction.protocol.version=2 // Throws error if metadata version > is < > > > 16, and this would be an upgrade > > > But I have updated the KIP to explicitly say the text you mentioned. > > > > > > Justine > > > > > > On Wed, Mar 27, 2024 at 1:41 PM José Armando García Sancio > > > wrote: > > > > > > > Hi Justine, > > > > > > > > See my comment below. > > > > > > > > On Wed, Mar 27, 2024 at 1:31 PM Justine Olshan > > > > wrote: > > > > > The feature command includes the upgrade or downgrade command along > > > with > > > > > the --release-version flag. If some features are not moving in the > > > > > direction mentioned (upgrade or downgrade) the command will fail -- > > > > perhaps > > > > > with an error of which features were going in the wrong direction. > > > > > > > > How about updating the KIP to show and document this behavior? > > > > > > > > Thanks, > > > > -- > > > > -José > > > > > > > > > >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Ok. I can remove the info from the describe output. Dependencies is needed for the storage tool because we want to make sure the desired versions we are setting will be valid. Version mapping should be for both tools since we have --release-version for both tools. I was considering changing the IV strings, but I wasn't sure if there would be some disagreement with the decision. Not sure if that breaks compatibility etc. Happy to hear everyone's thoughts. Justine On Wed, Mar 27, 2024 at 3:36 PM Jun Rao wrote: > Hi, Justine, > > Thanks for the reply. > > Having "kafka-feature dependencies" seems enough to me. We don't need to > include the dependencies in the output of "kafka-feature describe". > > We only support "dependencies" in kafka-feature, not kafka-storage. We > probably should do the same for "version-mapping". > > bin/kafka-features.sh downgrade --feature metadata.version=16 > --transaction.protocol.version=2 > We need to add the --feature flag for the second feature, right? > > In "kafka-features.sh describe", we only show the IV string for > metadata.version. Should we also show the level number? > > Thanks, > > Jun > > On Wed, Mar 27, 2024 at 1:52 PM Justine Olshan > > wrote: > > > I had already included this example > > bin/kafka-features.sh downgrade --feature metadata.version=16 > > --transaction.protocol.version=2 // Throws error if metadata version is < > > 16, and this would be an upgrade > > But I have updated the KIP to explicitly say the text you mentioned. > > > > Justine > > > > On Wed, Mar 27, 2024 at 1:41 PM José Armando García Sancio > > wrote: > > > > > Hi Justine, > > > > > > See my comment below. > > > > > > On Wed, Mar 27, 2024 at 1:31 PM Justine Olshan > > > wrote: > > > > The feature command includes the upgrade or downgrade command along > > with > > > > the --release-version flag. If some features are not moving in the > > > > direction mentioned (upgrade or downgrade) the command will fail -- > > > perhaps > > > > with an error of which features were going in the wrong direction. > > > > > > How about updating the KIP to show and document this behavior? > > > > > > Thanks, > > > -- > > > -José > > > > > >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi, Justine, Thanks for the reply. Having "kafka-feature dependencies" seems enough to me. We don't need to include the dependencies in the output of "kafka-feature describe". We only support "dependencies" in kafka-feature, not kafka-storage. We probably should do the same for "version-mapping". bin/kafka-features.sh downgrade --feature metadata.version=16 --transaction.protocol.version=2 We need to add the --feature flag for the second feature, right? In "kafka-features.sh describe", we only show the IV string for metadata.version. Should we also show the level number? Thanks, Jun On Wed, Mar 27, 2024 at 1:52 PM Justine Olshan wrote: > I had already included this example > bin/kafka-features.sh downgrade --feature metadata.version=16 > --transaction.protocol.version=2 // Throws error if metadata version is < > 16, and this would be an upgrade > But I have updated the KIP to explicitly say the text you mentioned. > > Justine > > On Wed, Mar 27, 2024 at 1:41 PM José Armando García Sancio > wrote: > > > Hi Justine, > > > > See my comment below. > > > > On Wed, Mar 27, 2024 at 1:31 PM Justine Olshan > > wrote: > > > The feature command includes the upgrade or downgrade command along > with > > > the --release-version flag. If some features are not moving in the > > > direction mentioned (upgrade or downgrade) the command will fail -- > > perhaps > > > with an error of which features were going in the wrong direction. > > > > How about updating the KIP to show and document this behavior? > > > > Thanks, > > -- > > -José > > >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
I had already included this example bin/kafka-features.sh downgrade --feature metadata.version=16 --transaction.protocol.version=2 // Throws error if metadata version is < 16, and this would be an upgrade But I have updated the KIP to explicitly say the text you mentioned. Justine On Wed, Mar 27, 2024 at 1:41 PM José Armando García Sancio wrote: > Hi Justine, > > See my comment below. > > On Wed, Mar 27, 2024 at 1:31 PM Justine Olshan > wrote: > > The feature command includes the upgrade or downgrade command along with > > the --release-version flag. If some features are not moving in the > > direction mentioned (upgrade or downgrade) the command will fail -- > perhaps > > with an error of which features were going in the wrong direction. > > How about updating the KIP to show and document this behavior? > > Thanks, > -- > -José >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi Justine, See my comment below. On Wed, Mar 27, 2024 at 1:31 PM Justine Olshan wrote: > The feature command includes the upgrade or downgrade command along with > the --release-version flag. If some features are not moving in the > direction mentioned (upgrade or downgrade) the command will fail -- perhaps > with an error of which features were going in the wrong direction. How about updating the KIP to show and document this behavior? Thanks, -- -José
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hey Jun, Right, sorry this would be one row in an output of all the various features (transaction.protocol.version, group.coordinator.version) currently set on the cluster. If we want to know the dependencies for each of a given feature (ie transaction.protocol.versions 0, 1, 2, 3 etc) we can use the other command if those versions are not yet set. If this makes sense I will update the KIP. I think one remaining open area was with respect to `--release-version` I didn't follow Colin's point > So how does the command invoking --release-version know whether it's upgrading or downgrading? The feature command includes the upgrade or downgrade command along with the --release-version flag. If some features are not moving in the direction mentioned (upgrade or downgrade) the command will fail -- perhaps with an error of which features were going in the wrong direction. Thanks, Justine On Wed, Mar 27, 2024 at 11:52 AM Jun Rao wrote: > Hi, Justine, > > It seems that we need to specify the dependencies for each feature version? > > Thanks, > > Jun > > On Wed, Mar 27, 2024 at 11:28 AM Justine Olshan > wrote: > > > Hey Jun, > > > > So just including the dependencies for the currently set features? Along > > with the supported min, max, and finalized versions? > > > > Feature: transaction.protocol.version SupportedMinVersion: 0 > > SupportedMaxVersion: 2 FinalizedVersionLevel: 1 Epoch: 3 Dependencies: > > metadata.version=4 > > > > On Wed, Mar 27, 2024 at 11:14 AM Jun Rao > wrote: > > > > > Hi, Justine, > > > > > > Yes, something like that. We could also extend "kafka-feature describe" > > by > > > adding the dependency to every feature in the output. > > > > > > Thanks, > > > > > > Jun > > > > > > On Wed, Mar 27, 2024 at 10:39 AM Justine Olshan > > > wrote: > > > > > > > Hi Jun, > > > > > > > > We could expose them in a tool. I'm wondering, are you thinking > > something > > > > like a command that lists the dependencies for a given feature + > > version? > > > > > > > > Something like: > > > > kafka-feature dependencies --feature transaction.protocol.version=2 > > > > > transaction.protocol.verison=2 requires metadata.version=4 (listing > > any > > > > other version dependencies) > > > > > > > > Justine > > > > > > > > On Wed, Mar 27, 2024 at 10:28 AM Jun Rao > > > wrote: > > > > > > > > > Hi, Colin, > > > > > > > > > > Thanks for the comments. It's fine if we want to keep the > --metadata > > > flag > > > > > if it's useful. Then we should add the same flag for kafka-storage > > for > > > > > consistency, right? > > > > > > > > > > Hi, Justine, > > > > > > > > > > Thanks for the reply. > > > > > > > > > > How will a user know about the dependencies among features? Should > we > > > > > expose them in a tool? > > > > > > > > > > Thanks, > > > > > > > > > > Jun > > > > > > > > > > On Tue, Mar 26, 2024 at 4:33 PM Colin McCabe > > > wrote: > > > > > > > > > > > Hi all, > > > > > > > > > > > > Thanks for the discussion. Let me respond to a few questions I > saw > > in > > > > the > > > > > > thread (hope I didn't miss any!) > > > > > > > > > > > > > > > > > > > > > > > > Feature level 0 is "special" because it effectively means that > the > > > > > feature > > > > > > hasn't been set up. So I could create a foo feature, a bar > feature, > > > > and a > > > > > > baz feature tomorrow and correctly say that your cluster is on > > > feature > > > > > > level 0 for foo, bar, and baz. Because feature level 0 is > > isomorphic > > > > with > > > > > > "no feature level set." > > > > > > > > > > > > Obviously you can have whatever semantics you want for feature > > level > > > 0. > > > > > In > > > > > > the case of Jose's Raft changes, feature level 0 may end up being > > the > > > > > > current state of the world. That's fine. > > > > > > > > > > > > 0 being isomorphic with "not set" simplifes the code a lot > because > > we > > > > > > don't need tons of special cases for "feature not set" versus > > > "feature > > > > > set > > > > > > to 0". Effectively we can use short integers everywhere, and not > > > > > > Optional. Does that make sense? > > > > > > > > > > > > > > > > > > > > > > > > The --metadata flag doesn't quite do the same thing as the > > --feature > > > > > flag. > > > > > > The --metadata flag takes a string like 3.7-IV0, whereas the > > > --feature > > > > > flag > > > > > > takes an integer like "17". > > > > > > > > > > > > It's true that in the current kafka-features.sh, you can shun the > > > > > > --metadata flag, and only use --feature. The --metadata flag is a > > > > > > convenience. But... conveniences are good. Better than > > > inconveniences. > > > > > So I > > > > > > don't think it makes sense to deprecate --metadata, since its > > > > > functionality > > > > > > is not provided by anything else. > > > > > > > > > > > > > > > > > > > > > > > > As I said earlier, the proposed semantics for --release-version > > > aren't > > > > > > actually
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi, Justine, It seems that we need to specify the dependencies for each feature version? Thanks, Jun On Wed, Mar 27, 2024 at 11:28 AM Justine Olshan wrote: > Hey Jun, > > So just including the dependencies for the currently set features? Along > with the supported min, max, and finalized versions? > > Feature: transaction.protocol.version SupportedMinVersion: 0 > SupportedMaxVersion: 2 FinalizedVersionLevel: 1 Epoch: 3 Dependencies: > metadata.version=4 > > On Wed, Mar 27, 2024 at 11:14 AM Jun Rao wrote: > > > Hi, Justine, > > > > Yes, something like that. We could also extend "kafka-feature describe" > by > > adding the dependency to every feature in the output. > > > > Thanks, > > > > Jun > > > > On Wed, Mar 27, 2024 at 10:39 AM Justine Olshan > > wrote: > > > > > Hi Jun, > > > > > > We could expose them in a tool. I'm wondering, are you thinking > something > > > like a command that lists the dependencies for a given feature + > version? > > > > > > Something like: > > > kafka-feature dependencies --feature transaction.protocol.version=2 > > > > transaction.protocol.verison=2 requires metadata.version=4 (listing > any > > > other version dependencies) > > > > > > Justine > > > > > > On Wed, Mar 27, 2024 at 10:28 AM Jun Rao > > wrote: > > > > > > > Hi, Colin, > > > > > > > > Thanks for the comments. It's fine if we want to keep the --metadata > > flag > > > > if it's useful. Then we should add the same flag for kafka-storage > for > > > > consistency, right? > > > > > > > > Hi, Justine, > > > > > > > > Thanks for the reply. > > > > > > > > How will a user know about the dependencies among features? Should we > > > > expose them in a tool? > > > > > > > > Thanks, > > > > > > > > Jun > > > > > > > > On Tue, Mar 26, 2024 at 4:33 PM Colin McCabe > > wrote: > > > > > > > > > Hi all, > > > > > > > > > > Thanks for the discussion. Let me respond to a few questions I saw > in > > > the > > > > > thread (hope I didn't miss any!) > > > > > > > > > > > > > > > > > > > > Feature level 0 is "special" because it effectively means that the > > > > feature > > > > > hasn't been set up. So I could create a foo feature, a bar feature, > > > and a > > > > > baz feature tomorrow and correctly say that your cluster is on > > feature > > > > > level 0 for foo, bar, and baz. Because feature level 0 is > isomorphic > > > with > > > > > "no feature level set." > > > > > > > > > > Obviously you can have whatever semantics you want for feature > level > > 0. > > > > In > > > > > the case of Jose's Raft changes, feature level 0 may end up being > the > > > > > current state of the world. That's fine. > > > > > > > > > > 0 being isomorphic with "not set" simplifes the code a lot because > we > > > > > don't need tons of special cases for "feature not set" versus > > "feature > > > > set > > > > > to 0". Effectively we can use short integers everywhere, and not > > > > > Optional. Does that make sense? > > > > > > > > > > > > > > > > > > > > The --metadata flag doesn't quite do the same thing as the > --feature > > > > flag. > > > > > The --metadata flag takes a string like 3.7-IV0, whereas the > > --feature > > > > flag > > > > > takes an integer like "17". > > > > > > > > > > It's true that in the current kafka-features.sh, you can shun the > > > > > --metadata flag, and only use --feature. The --metadata flag is a > > > > > convenience. But... conveniences are good. Better than > > inconveniences. > > > > So I > > > > > don't think it makes sense to deprecate --metadata, since its > > > > functionality > > > > > is not provided by anything else. > > > > > > > > > > > > > > > > > > > > As I said earlier, the proposed semantics for --release-version > > aren't > > > > > actually possible given the current RPCs on the server side. The > > > problem > > > > is > > > > > that UpdateFeaturesRequest needs to be set upgradType to one of: > > > > > > > > > > 1. FeatureUpdate.UpgradeType.UPGRADE > > > > > 2. FeatureUpdate.UpgradeType.SAFE_DOWNGRADE > > > > > 3. FeatureUpdate.UpgradeType.UNSAFE_DOWNGRADE > > > > > > > > > > If it's set to #1, levels can only go up; if it's set to 2 or 3, > > levels > > > > > can only go down. (I forget what happens if the levels are the > > same... > > > > you > > > > > can check) > > > > > > > > > > So how does the command invoking --release-version know whether > it's > > > > > upgrading or downgrading? I can't see any way for it to know, and > > plus > > > it > > > > > may end up doing more than one of these if some features need to go > > > down > > > > > and others up. "Making everything the same as it was in 3.7-IV0" > may > > > end > > > > up > > > > > down-levelling some features, and up-levelling others. There isn't > > any > > > > way > > > > > to do this atomically in a single RPC today. > > > > > > > > > > > > > > > > > > > > I don't find the proposed semantics for --release-version to be > very > > > > > useful. > > > > > > > > > >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hey Jun, So just including the dependencies for the currently set features? Along with the supported min, max, and finalized versions? Feature: transaction.protocol.version SupportedMinVersion: 0 SupportedMaxVersion: 2 FinalizedVersionLevel: 1 Epoch: 3 Dependencies: metadata.version=4 On Wed, Mar 27, 2024 at 11:14 AM Jun Rao wrote: > Hi, Justine, > > Yes, something like that. We could also extend "kafka-feature describe" by > adding the dependency to every feature in the output. > > Thanks, > > Jun > > On Wed, Mar 27, 2024 at 10:39 AM Justine Olshan > wrote: > > > Hi Jun, > > > > We could expose them in a tool. I'm wondering, are you thinking something > > like a command that lists the dependencies for a given feature + version? > > > > Something like: > > kafka-feature dependencies --feature transaction.protocol.version=2 > > > transaction.protocol.verison=2 requires metadata.version=4 (listing any > > other version dependencies) > > > > Justine > > > > On Wed, Mar 27, 2024 at 10:28 AM Jun Rao > wrote: > > > > > Hi, Colin, > > > > > > Thanks for the comments. It's fine if we want to keep the --metadata > flag > > > if it's useful. Then we should add the same flag for kafka-storage for > > > consistency, right? > > > > > > Hi, Justine, > > > > > > Thanks for the reply. > > > > > > How will a user know about the dependencies among features? Should we > > > expose them in a tool? > > > > > > Thanks, > > > > > > Jun > > > > > > On Tue, Mar 26, 2024 at 4:33 PM Colin McCabe > wrote: > > > > > > > Hi all, > > > > > > > > Thanks for the discussion. Let me respond to a few questions I saw in > > the > > > > thread (hope I didn't miss any!) > > > > > > > > > > > > > > > > Feature level 0 is "special" because it effectively means that the > > > feature > > > > hasn't been set up. So I could create a foo feature, a bar feature, > > and a > > > > baz feature tomorrow and correctly say that your cluster is on > feature > > > > level 0 for foo, bar, and baz. Because feature level 0 is isomorphic > > with > > > > "no feature level set." > > > > > > > > Obviously you can have whatever semantics you want for feature level > 0. > > > In > > > > the case of Jose's Raft changes, feature level 0 may end up being the > > > > current state of the world. That's fine. > > > > > > > > 0 being isomorphic with "not set" simplifes the code a lot because we > > > > don't need tons of special cases for "feature not set" versus > "feature > > > set > > > > to 0". Effectively we can use short integers everywhere, and not > > > > Optional. Does that make sense? > > > > > > > > > > > > > > > > The --metadata flag doesn't quite do the same thing as the --feature > > > flag. > > > > The --metadata flag takes a string like 3.7-IV0, whereas the > --feature > > > flag > > > > takes an integer like "17". > > > > > > > > It's true that in the current kafka-features.sh, you can shun the > > > > --metadata flag, and only use --feature. The --metadata flag is a > > > > convenience. But... conveniences are good. Better than > inconveniences. > > > So I > > > > don't think it makes sense to deprecate --metadata, since its > > > functionality > > > > is not provided by anything else. > > > > > > > > > > > > > > > > As I said earlier, the proposed semantics for --release-version > aren't > > > > actually possible given the current RPCs on the server side. The > > problem > > > is > > > > that UpdateFeaturesRequest needs to be set upgradType to one of: > > > > > > > > 1. FeatureUpdate.UpgradeType.UPGRADE > > > > 2. FeatureUpdate.UpgradeType.SAFE_DOWNGRADE > > > > 3. FeatureUpdate.UpgradeType.UNSAFE_DOWNGRADE > > > > > > > > If it's set to #1, levels can only go up; if it's set to 2 or 3, > levels > > > > can only go down. (I forget what happens if the levels are the > same... > > > you > > > > can check) > > > > > > > > So how does the command invoking --release-version know whether it's > > > > upgrading or downgrading? I can't see any way for it to know, and > plus > > it > > > > may end up doing more than one of these if some features need to go > > down > > > > and others up. "Making everything the same as it was in 3.7-IV0" may > > end > > > up > > > > down-levelling some features, and up-levelling others. There isn't > any > > > way > > > > to do this atomically in a single RPC today. > > > > > > > > > > > > > > > > I don't find the proposed semantics for --release-version to be very > > > > useful. > > > > > > > > In the clusters I help to administer, I don't like changing a bunch > of > > > > things all at once. I'd much rather make one change at a time and see > > > what > > > > happens, then move on to the next change. > > > > > > > > Earlier I proposed just having a subcommand in kafka-features.sh that > > > > compared the currently set feature flags against the "default" one > for > > > the > > > > provided Kafka release / MV. I think this would be more useful than > the > > > >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi, Justine, Yes, something like that. We could also extend "kafka-feature describe" by adding the dependency to every feature in the output. Thanks, Jun On Wed, Mar 27, 2024 at 10:39 AM Justine Olshan wrote: > Hi Jun, > > We could expose them in a tool. I'm wondering, are you thinking something > like a command that lists the dependencies for a given feature + version? > > Something like: > kafka-feature dependencies --feature transaction.protocol.version=2 > > transaction.protocol.verison=2 requires metadata.version=4 (listing any > other version dependencies) > > Justine > > On Wed, Mar 27, 2024 at 10:28 AM Jun Rao wrote: > > > Hi, Colin, > > > > Thanks for the comments. It's fine if we want to keep the --metadata flag > > if it's useful. Then we should add the same flag for kafka-storage for > > consistency, right? > > > > Hi, Justine, > > > > Thanks for the reply. > > > > How will a user know about the dependencies among features? Should we > > expose them in a tool? > > > > Thanks, > > > > Jun > > > > On Tue, Mar 26, 2024 at 4:33 PM Colin McCabe wrote: > > > > > Hi all, > > > > > > Thanks for the discussion. Let me respond to a few questions I saw in > the > > > thread (hope I didn't miss any!) > > > > > > > > > > > > Feature level 0 is "special" because it effectively means that the > > feature > > > hasn't been set up. So I could create a foo feature, a bar feature, > and a > > > baz feature tomorrow and correctly say that your cluster is on feature > > > level 0 for foo, bar, and baz. Because feature level 0 is isomorphic > with > > > "no feature level set." > > > > > > Obviously you can have whatever semantics you want for feature level 0. > > In > > > the case of Jose's Raft changes, feature level 0 may end up being the > > > current state of the world. That's fine. > > > > > > 0 being isomorphic with "not set" simplifes the code a lot because we > > > don't need tons of special cases for "feature not set" versus "feature > > set > > > to 0". Effectively we can use short integers everywhere, and not > > > Optional. Does that make sense? > > > > > > > > > > > > The --metadata flag doesn't quite do the same thing as the --feature > > flag. > > > The --metadata flag takes a string like 3.7-IV0, whereas the --feature > > flag > > > takes an integer like "17". > > > > > > It's true that in the current kafka-features.sh, you can shun the > > > --metadata flag, and only use --feature. The --metadata flag is a > > > convenience. But... conveniences are good. Better than inconveniences. > > So I > > > don't think it makes sense to deprecate --metadata, since its > > functionality > > > is not provided by anything else. > > > > > > > > > > > > As I said earlier, the proposed semantics for --release-version aren't > > > actually possible given the current RPCs on the server side. The > problem > > is > > > that UpdateFeaturesRequest needs to be set upgradType to one of: > > > > > > 1. FeatureUpdate.UpgradeType.UPGRADE > > > 2. FeatureUpdate.UpgradeType.SAFE_DOWNGRADE > > > 3. FeatureUpdate.UpgradeType.UNSAFE_DOWNGRADE > > > > > > If it's set to #1, levels can only go up; if it's set to 2 or 3, levels > > > can only go down. (I forget what happens if the levels are the same... > > you > > > can check) > > > > > > So how does the command invoking --release-version know whether it's > > > upgrading or downgrading? I can't see any way for it to know, and plus > it > > > may end up doing more than one of these if some features need to go > down > > > and others up. "Making everything the same as it was in 3.7-IV0" may > end > > up > > > down-levelling some features, and up-levelling others. There isn't any > > way > > > to do this atomically in a single RPC today. > > > > > > > > > > > > I don't find the proposed semantics for --release-version to be very > > > useful. > > > > > > In the clusters I help to administer, I don't like changing a bunch of > > > things all at once. I'd much rather make one change at a time and see > > what > > > happens, then move on to the next change. > > > > > > Earlier I proposed just having a subcommand in kafka-features.sh that > > > compared the currently set feature flags against the "default" one for > > the > > > provided Kafka release / MV. I think this would be more useful than the > > > "shotgun approach" of making a bunch of changes together. Just DISPLAY > > what > > > would need to be changed to "make everything the same as it was in > > 3.7-IV0" > > > but then let the admin decide what they want to do (or not do). You > could > > > even display the commands that would need to be run, if you like. But > let > > > them decide whether to pull the trigger on each upgrade or downgrade. > > > > > > This also avoids having to solve the thorny issue of how to have a > single > > > RPC do both upgrades and downgrades. > > > > > > best, > > > Colin > > > > > > > > > On Tue, Mar 26, 2024, at 14:59, Justine Olshan
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi Jun, We could expose them in a tool. I'm wondering, are you thinking something like a command that lists the dependencies for a given feature + version? Something like: kafka-feature dependencies --feature transaction.protocol.version=2 > transaction.protocol.verison=2 requires metadata.version=4 (listing any other version dependencies) Justine On Wed, Mar 27, 2024 at 10:28 AM Jun Rao wrote: > Hi, Colin, > > Thanks for the comments. It's fine if we want to keep the --metadata flag > if it's useful. Then we should add the same flag for kafka-storage for > consistency, right? > > Hi, Justine, > > Thanks for the reply. > > How will a user know about the dependencies among features? Should we > expose them in a tool? > > Thanks, > > Jun > > On Tue, Mar 26, 2024 at 4:33 PM Colin McCabe wrote: > > > Hi all, > > > > Thanks for the discussion. Let me respond to a few questions I saw in the > > thread (hope I didn't miss any!) > > > > > > > > Feature level 0 is "special" because it effectively means that the > feature > > hasn't been set up. So I could create a foo feature, a bar feature, and a > > baz feature tomorrow and correctly say that your cluster is on feature > > level 0 for foo, bar, and baz. Because feature level 0 is isomorphic with > > "no feature level set." > > > > Obviously you can have whatever semantics you want for feature level 0. > In > > the case of Jose's Raft changes, feature level 0 may end up being the > > current state of the world. That's fine. > > > > 0 being isomorphic with "not set" simplifes the code a lot because we > > don't need tons of special cases for "feature not set" versus "feature > set > > to 0". Effectively we can use short integers everywhere, and not > > Optional. Does that make sense? > > > > > > > > The --metadata flag doesn't quite do the same thing as the --feature > flag. > > The --metadata flag takes a string like 3.7-IV0, whereas the --feature > flag > > takes an integer like "17". > > > > It's true that in the current kafka-features.sh, you can shun the > > --metadata flag, and only use --feature. The --metadata flag is a > > convenience. But... conveniences are good. Better than inconveniences. > So I > > don't think it makes sense to deprecate --metadata, since its > functionality > > is not provided by anything else. > > > > > > > > As I said earlier, the proposed semantics for --release-version aren't > > actually possible given the current RPCs on the server side. The problem > is > > that UpdateFeaturesRequest needs to be set upgradType to one of: > > > > 1. FeatureUpdate.UpgradeType.UPGRADE > > 2. FeatureUpdate.UpgradeType.SAFE_DOWNGRADE > > 3. FeatureUpdate.UpgradeType.UNSAFE_DOWNGRADE > > > > If it's set to #1, levels can only go up; if it's set to 2 or 3, levels > > can only go down. (I forget what happens if the levels are the same... > you > > can check) > > > > So how does the command invoking --release-version know whether it's > > upgrading or downgrading? I can't see any way for it to know, and plus it > > may end up doing more than one of these if some features need to go down > > and others up. "Making everything the same as it was in 3.7-IV0" may end > up > > down-levelling some features, and up-levelling others. There isn't any > way > > to do this atomically in a single RPC today. > > > > > > > > I don't find the proposed semantics for --release-version to be very > > useful. > > > > In the clusters I help to administer, I don't like changing a bunch of > > things all at once. I'd much rather make one change at a time and see > what > > happens, then move on to the next change. > > > > Earlier I proposed just having a subcommand in kafka-features.sh that > > compared the currently set feature flags against the "default" one for > the > > provided Kafka release / MV. I think this would be more useful than the > > "shotgun approach" of making a bunch of changes together. Just DISPLAY > what > > would need to be changed to "make everything the same as it was in > 3.7-IV0" > > but then let the admin decide what they want to do (or not do). You could > > even display the commands that would need to be run, if you like. But let > > them decide whether to pull the trigger on each upgrade or downgrade. > > > > This also avoids having to solve the thorny issue of how to have a single > > RPC do both upgrades and downgrades. > > > > best, > > Colin > > > > > > On Tue, Mar 26, 2024, at 14:59, Justine Olshan wrote: > > > Hi all, > > > > > > I've added the notes about requiring 3.3-IV0 and that the tool/rpc will > > > fail if the metadata version is too low. > > > > > > I will wait for Colin's response on the deprecation. I am not opposed > to > > > deprecating it but want everyone to agree. > > > > > > Justine > > > > > > On Tue, Mar 26, 2024 at 12:26 PM José Armando García Sancio > > > wrote: > > > > > >> Hi Justine, > > >> > > >> On Mon, Mar 25, 2024 at 5:09 PM Justine Olshan > > >>
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi, Colin, Thanks for the comments. It's fine if we want to keep the --metadata flag if it's useful. Then we should add the same flag for kafka-storage for consistency, right? Hi, Justine, Thanks for the reply. How will a user know about the dependencies among features? Should we expose them in a tool? Thanks, Jun On Tue, Mar 26, 2024 at 4:33 PM Colin McCabe wrote: > Hi all, > > Thanks for the discussion. Let me respond to a few questions I saw in the > thread (hope I didn't miss any!) > > > > Feature level 0 is "special" because it effectively means that the feature > hasn't been set up. So I could create a foo feature, a bar feature, and a > baz feature tomorrow and correctly say that your cluster is on feature > level 0 for foo, bar, and baz. Because feature level 0 is isomorphic with > "no feature level set." > > Obviously you can have whatever semantics you want for feature level 0. In > the case of Jose's Raft changes, feature level 0 may end up being the > current state of the world. That's fine. > > 0 being isomorphic with "not set" simplifes the code a lot because we > don't need tons of special cases for "feature not set" versus "feature set > to 0". Effectively we can use short integers everywhere, and not > Optional. Does that make sense? > > > > The --metadata flag doesn't quite do the same thing as the --feature flag. > The --metadata flag takes a string like 3.7-IV0, whereas the --feature flag > takes an integer like "17". > > It's true that in the current kafka-features.sh, you can shun the > --metadata flag, and only use --feature. The --metadata flag is a > convenience. But... conveniences are good. Better than inconveniences. So I > don't think it makes sense to deprecate --metadata, since its functionality > is not provided by anything else. > > > > As I said earlier, the proposed semantics for --release-version aren't > actually possible given the current RPCs on the server side. The problem is > that UpdateFeaturesRequest needs to be set upgradType to one of: > > 1. FeatureUpdate.UpgradeType.UPGRADE > 2. FeatureUpdate.UpgradeType.SAFE_DOWNGRADE > 3. FeatureUpdate.UpgradeType.UNSAFE_DOWNGRADE > > If it's set to #1, levels can only go up; if it's set to 2 or 3, levels > can only go down. (I forget what happens if the levels are the same... you > can check) > > So how does the command invoking --release-version know whether it's > upgrading or downgrading? I can't see any way for it to know, and plus it > may end up doing more than one of these if some features need to go down > and others up. "Making everything the same as it was in 3.7-IV0" may end up > down-levelling some features, and up-levelling others. There isn't any way > to do this atomically in a single RPC today. > > > > I don't find the proposed semantics for --release-version to be very > useful. > > In the clusters I help to administer, I don't like changing a bunch of > things all at once. I'd much rather make one change at a time and see what > happens, then move on to the next change. > > Earlier I proposed just having a subcommand in kafka-features.sh that > compared the currently set feature flags against the "default" one for the > provided Kafka release / MV. I think this would be more useful than the > "shotgun approach" of making a bunch of changes together. Just DISPLAY what > would need to be changed to "make everything the same as it was in 3.7-IV0" > but then let the admin decide what they want to do (or not do). You could > even display the commands that would need to be run, if you like. But let > them decide whether to pull the trigger on each upgrade or downgrade. > > This also avoids having to solve the thorny issue of how to have a single > RPC do both upgrades and downgrades. > > best, > Colin > > > On Tue, Mar 26, 2024, at 14:59, Justine Olshan wrote: > > Hi all, > > > > I've added the notes about requiring 3.3-IV0 and that the tool/rpc will > > fail if the metadata version is too low. > > > > I will wait for Colin's response on the deprecation. I am not opposed to > > deprecating it but want everyone to agree. > > > > Justine > > > > On Tue, Mar 26, 2024 at 12:26 PM José Armando García Sancio > > wrote: > > > >> Hi Justine, > >> > >> On Mon, Mar 25, 2024 at 5:09 PM Justine Olshan > >> wrote: > >> > The reason it is not removed is purely for backwards > >> > compatibility. Colin had strong feelings about not removing any flags. > >> > >> We are not saying that we should remove that flag. That would break > >> backward compatibility of 3.8 with 3.7. We are suggesting to deprecate > >> the flag in the next release. > >> > >> Thanks, > >> -José > >> >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi all, Thanks for the discussion. Let me respond to a few questions I saw in the thread (hope I didn't miss any!) Feature level 0 is "special" because it effectively means that the feature hasn't been set up. So I could create a foo feature, a bar feature, and a baz feature tomorrow and correctly say that your cluster is on feature level 0 for foo, bar, and baz. Because feature level 0 is isomorphic with "no feature level set." Obviously you can have whatever semantics you want for feature level 0. In the case of Jose's Raft changes, feature level 0 may end up being the current state of the world. That's fine. 0 being isomorphic with "not set" simplifes the code a lot because we don't need tons of special cases for "feature not set" versus "feature set to 0". Effectively we can use short integers everywhere, and not Optional. Does that make sense? The --metadata flag doesn't quite do the same thing as the --feature flag. The --metadata flag takes a string like 3.7-IV0, whereas the --feature flag takes an integer like "17". It's true that in the current kafka-features.sh, you can shun the --metadata flag, and only use --feature. The --metadata flag is a convenience. But... conveniences are good. Better than inconveniences. So I don't think it makes sense to deprecate --metadata, since its functionality is not provided by anything else. As I said earlier, the proposed semantics for --release-version aren't actually possible given the current RPCs on the server side. The problem is that UpdateFeaturesRequest needs to be set upgradType to one of: 1. FeatureUpdate.UpgradeType.UPGRADE 2. FeatureUpdate.UpgradeType.SAFE_DOWNGRADE 3. FeatureUpdate.UpgradeType.UNSAFE_DOWNGRADE If it's set to #1, levels can only go up; if it's set to 2 or 3, levels can only go down. (I forget what happens if the levels are the same... you can check) So how does the command invoking --release-version know whether it's upgrading or downgrading? I can't see any way for it to know, and plus it may end up doing more than one of these if some features need to go down and others up. "Making everything the same as it was in 3.7-IV0" may end up down-levelling some features, and up-levelling others. There isn't any way to do this atomically in a single RPC today. I don't find the proposed semantics for --release-version to be very useful. In the clusters I help to administer, I don't like changing a bunch of things all at once. I'd much rather make one change at a time and see what happens, then move on to the next change. Earlier I proposed just having a subcommand in kafka-features.sh that compared the currently set feature flags against the "default" one for the provided Kafka release / MV. I think this would be more useful than the "shotgun approach" of making a bunch of changes together. Just DISPLAY what would need to be changed to "make everything the same as it was in 3.7-IV0" but then let the admin decide what they want to do (or not do). You could even display the commands that would need to be run, if you like. But let them decide whether to pull the trigger on each upgrade or downgrade. This also avoids having to solve the thorny issue of how to have a single RPC do both upgrades and downgrades. best, Colin On Tue, Mar 26, 2024, at 14:59, Justine Olshan wrote: > Hi all, > > I've added the notes about requiring 3.3-IV0 and that the tool/rpc will > fail if the metadata version is too low. > > I will wait for Colin's response on the deprecation. I am not opposed to > deprecating it but want everyone to agree. > > Justine > > On Tue, Mar 26, 2024 at 12:26 PM José Armando García Sancio > wrote: > >> Hi Justine, >> >> On Mon, Mar 25, 2024 at 5:09 PM Justine Olshan >> wrote: >> > The reason it is not removed is purely for backwards >> > compatibility. Colin had strong feelings about not removing any flags. >> >> We are not saying that we should remove that flag. That would break >> backward compatibility of 3.8 with 3.7. We are suggesting to deprecate >> the flag in the next release. >> >> Thanks, >> -José >>
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi all, I've added the notes about requiring 3.3-IV0 and that the tool/rpc will fail if the metadata version is too low. I will wait for Colin's response on the deprecation. I am not opposed to deprecating it but want everyone to agree. Justine On Tue, Mar 26, 2024 at 12:26 PM José Armando García Sancio wrote: > Hi Justine, > > On Mon, Mar 25, 2024 at 5:09 PM Justine Olshan > wrote: > > The reason it is not removed is purely for backwards > > compatibility. Colin had strong feelings about not removing any flags. > > We are not saying that we should remove that flag. That would break > backward compatibility of 3.8 with 3.7. We are suggesting to deprecate > the flag in the next release. > > Thanks, > -José >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi Justine, On Mon, Mar 25, 2024 at 5:09 PM Justine Olshan wrote: > The reason it is not removed is purely for backwards > compatibility. Colin had strong feelings about not removing any flags. We are not saying that we should remove that flag. That would break backward compatibility of 3.8 with 3.7. We are suggesting to deprecate the flag in the next release. Thanks, -José
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi, Justine, Thanks for the reply. 10. If we are exposing group.coordinator.version in this KIP, I think we need to document what RPCs/records it controls or if it has dependencies on MV. 15. "For transaction version specifically, I have no metadata version dependencies besides requiring 3.3 to write the feature records and use the feature tools. I would suspect all new features would have this requirement." Could we document the TV dependency on MV and that the tools/RPC will fail if the provided TV and MV version are not compatible? Hi, Colin, The functionality of --metadata METADATA flag in kafka-features can be achieved by the new --feature flag. Is there a need to still keep the --metadata flag? Thanks, Jun On Mon, Mar 25, 2024 at 5:09 PM Justine Olshan wrote: > Hi Jun, > > I apologize for typos. I thought I got rid of all the non protocol > versions. It is protocol version as per my previous email. I will fix the > KIP. > > The group coordinator version is used to upgrade to the new group > coordinator protocol. (KIP-848) > I don't have all the context there. > > I would prefer to not add the metadata flag to the storage tool as it is > not necessary. The reason it is not removed is purely for backwards > compatibility. Colin had strong feelings about not removing any flags. > > Justine > > On Mon, Mar 25, 2024 at 5:01 PM Jun Rao wrote: > > > Hi, Justine, > > > > Thanks for the updated KIP. A few more comments. > > > > 10. Could we describe what RPCs group.coordinator.version controls? > > > > 12. --metadata METADATA is not removed from kafka-features. Do we have a > > justification for this? If so, should we add that to kafka-storage to be > > consistent? > > > > 14. The KIP has both transaction.protocol.version vs transaction.version. > > What's the correct feature name? > > > > Jun > > > > On Mon, Mar 25, 2024 at 4:54 PM Justine Olshan > > > > wrote: > > > > > I've updated the KIP to include this CLI. For now, I've moved the new > api > > > to the server as a rejected alternative. > > > > > > It seems like we can keep the mapping in the tool. Given that we also > > need > > > to do the same validation for using multiple --feature flags (the > request > > > will look the same to the server), we can have the --release-version > > flag. > > > > > > I think that closes the main discussions. Please let me know if there > is > > > any further discussion I missed. > > > > > > Justine > > > > > > On Mon, Mar 25, 2024 at 4:44 PM Justine Olshan > > > wrote: > > > > > > > Hi Jose, > > > > > > > > Sorry for the typos. I think you figured out what I meant. > > > > > > > > I can make a new API. There is a risk of creating a ton of very > similar > > > > APIs though. Even the ApiVersions api is confusing with its supported > > and > > > > finalized features fields. I wonder if there is a middle ground here. > > > > I can have the storage tool and features tool rely on the feature and > > not > > > > query the server. Colin seemed to be against that. > > > > > Anyway your idea of putting the info on the server side is probably > > for > > > > the best. > > > > > > > > --release-version can work with the downgrade tool too. I just didn't > > > > think I needed to directly spell that out. I can though. > > > > > > > > I wish we weren't splitting this conversation among the two threads. > > :( I > > > > tried to get this out so it could cover all KIPs. Having this on > > separate > > > > threads makes getting this consensus even harder than it already is. > > > > From what I can tell your KIP's text matches this. Is the expectation > > > that > > > > the added flags will be done as part of this KIP or your KIP? I don't > > > > really have a strong opinion about --release-version so maybe it > should > > > > have been part of your KIP all along. > > > > > > > > > To me version 0 doesn't necessarily mean that the feature is not > > > > enabled. For example, for kraft.version, version 0 means the protocol > > > > prior to KIP-853. Version 0 is the currently implemented version of > > > > the KRaft protocol. > > > > > > > > This is what Colin told me in our previous discussions. I don't > really > > > > feel too strongly about the semantics here. > > > > > > > > So it seems like the only real undecided item here is whether we > should > > > > have this new api query the server or rely on the information being > > built > > > > in the tool. > > > > I will update the KIP to include the CLI command to get the > > information. > > > > > > > > Justine > > > > > > > > On Mon, Mar 25, 2024 at 4:19 PM José Armando García Sancio > > > > wrote: > > > > > > > >> Hi Justine, > > > >> > > > >> Thanks for the update. See my comments below. > > > >> > > > >> On Mon, Mar 25, 2024 at 2:51 PM Justine Olshan > > > >> wrote: > > > >> > I've updated the KIP with the changes I mentioned earlier. I have > > not > > > >> yet > > > >> > removed the --feature-version flag from the upgrade tool. > > > >> > > > >> What's the
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi Jun, I apologize for typos. I thought I got rid of all the non protocol versions. It is protocol version as per my previous email. I will fix the KIP. The group coordinator version is used to upgrade to the new group coordinator protocol. (KIP-848) I don't have all the context there. I would prefer to not add the metadata flag to the storage tool as it is not necessary. The reason it is not removed is purely for backwards compatibility. Colin had strong feelings about not removing any flags. Justine On Mon, Mar 25, 2024 at 5:01 PM Jun Rao wrote: > Hi, Justine, > > Thanks for the updated KIP. A few more comments. > > 10. Could we describe what RPCs group.coordinator.version controls? > > 12. --metadata METADATA is not removed from kafka-features. Do we have a > justification for this? If so, should we add that to kafka-storage to be > consistent? > > 14. The KIP has both transaction.protocol.version vs transaction.version. > What's the correct feature name? > > Jun > > On Mon, Mar 25, 2024 at 4:54 PM Justine Olshan > > wrote: > > > I've updated the KIP to include this CLI. For now, I've moved the new api > > to the server as a rejected alternative. > > > > It seems like we can keep the mapping in the tool. Given that we also > need > > to do the same validation for using multiple --feature flags (the request > > will look the same to the server), we can have the --release-version > flag. > > > > I think that closes the main discussions. Please let me know if there is > > any further discussion I missed. > > > > Justine > > > > On Mon, Mar 25, 2024 at 4:44 PM Justine Olshan > > wrote: > > > > > Hi Jose, > > > > > > Sorry for the typos. I think you figured out what I meant. > > > > > > I can make a new API. There is a risk of creating a ton of very similar > > > APIs though. Even the ApiVersions api is confusing with its supported > and > > > finalized features fields. I wonder if there is a middle ground here. > > > I can have the storage tool and features tool rely on the feature and > not > > > query the server. Colin seemed to be against that. > > > > Anyway your idea of putting the info on the server side is probably > for > > > the best. > > > > > > --release-version can work with the downgrade tool too. I just didn't > > > think I needed to directly spell that out. I can though. > > > > > > I wish we weren't splitting this conversation among the two threads. > :( I > > > tried to get this out so it could cover all KIPs. Having this on > separate > > > threads makes getting this consensus even harder than it already is. > > > From what I can tell your KIP's text matches this. Is the expectation > > that > > > the added flags will be done as part of this KIP or your KIP? I don't > > > really have a strong opinion about --release-version so maybe it should > > > have been part of your KIP all along. > > > > > > > To me version 0 doesn't necessarily mean that the feature is not > > > enabled. For example, for kraft.version, version 0 means the protocol > > > prior to KIP-853. Version 0 is the currently implemented version of > > > the KRaft protocol. > > > > > > This is what Colin told me in our previous discussions. I don't really > > > feel too strongly about the semantics here. > > > > > > So it seems like the only real undecided item here is whether we should > > > have this new api query the server or rely on the information being > built > > > in the tool. > > > I will update the KIP to include the CLI command to get the > information. > > > > > > Justine > > > > > > On Mon, Mar 25, 2024 at 4:19 PM José Armando García Sancio > > > wrote: > > > > > >> Hi Justine, > > >> > > >> Thanks for the update. See my comments below. > > >> > > >> On Mon, Mar 25, 2024 at 2:51 PM Justine Olshan > > >> wrote: > > >> > I've updated the KIP with the changes I mentioned earlier. I have > not > > >> yet > > >> > removed the --feature-version flag from the upgrade tool. > > >> > > >> What's the "--feature-version" flag? This is the first time I see it > > >> mentioned and I don't see it in the KIP. Did you mean > > >> "--release-version"? > > >> > > >> > Please take a look at the API to get the versions for a given > > >> > metadata version. Right now I'm using ApiVersions request and > > >> specifying a > > >> > metadata version. The supported versions are then supplied in the > > >> > ApiVersions response. > > >> > There were tradeoffs with this approach. It is a pretty minimal > > change, > > >> but > > >> > reusing the API means that it could be confusing (ie, the ApiKeys > will > > >> be > > >> > supplied in the response but not needed.) I considered making a > whole > > >> new > > >> > API, but that didn't seem necessary for this use. > > >> > > >> I agree that this is extremely confusing and we shouldn't overload the > > >> ApiVersions RPC to return this information. The KIP doesn't mention > > >> how it is going to use this API. Do you need to update the Admin > > >> client to include this
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi, Justine, Thanks for the updated KIP. A few more comments. 10. Could we describe what RPCs group.coordinator.version controls? 12. --metadata METADATA is not removed from kafka-features. Do we have a justification for this? If so, should we add that to kafka-storage to be consistent? 14. The KIP has both transaction.protocol.version vs transaction.version. What's the correct feature name? Jun On Mon, Mar 25, 2024 at 4:54 PM Justine Olshan wrote: > I've updated the KIP to include this CLI. For now, I've moved the new api > to the server as a rejected alternative. > > It seems like we can keep the mapping in the tool. Given that we also need > to do the same validation for using multiple --feature flags (the request > will look the same to the server), we can have the --release-version flag. > > I think that closes the main discussions. Please let me know if there is > any further discussion I missed. > > Justine > > On Mon, Mar 25, 2024 at 4:44 PM Justine Olshan > wrote: > > > Hi Jose, > > > > Sorry for the typos. I think you figured out what I meant. > > > > I can make a new API. There is a risk of creating a ton of very similar > > APIs though. Even the ApiVersions api is confusing with its supported and > > finalized features fields. I wonder if there is a middle ground here. > > I can have the storage tool and features tool rely on the feature and not > > query the server. Colin seemed to be against that. > > > Anyway your idea of putting the info on the server side is probably for > > the best. > > > > --release-version can work with the downgrade tool too. I just didn't > > think I needed to directly spell that out. I can though. > > > > I wish we weren't splitting this conversation among the two threads. :( I > > tried to get this out so it could cover all KIPs. Having this on separate > > threads makes getting this consensus even harder than it already is. > > From what I can tell your KIP's text matches this. Is the expectation > that > > the added flags will be done as part of this KIP or your KIP? I don't > > really have a strong opinion about --release-version so maybe it should > > have been part of your KIP all along. > > > > > To me version 0 doesn't necessarily mean that the feature is not > > enabled. For example, for kraft.version, version 0 means the protocol > > prior to KIP-853. Version 0 is the currently implemented version of > > the KRaft protocol. > > > > This is what Colin told me in our previous discussions. I don't really > > feel too strongly about the semantics here. > > > > So it seems like the only real undecided item here is whether we should > > have this new api query the server or rely on the information being built > > in the tool. > > I will update the KIP to include the CLI command to get the information. > > > > Justine > > > > On Mon, Mar 25, 2024 at 4:19 PM José Armando García Sancio > > wrote: > > > >> Hi Justine, > >> > >> Thanks for the update. See my comments below. > >> > >> On Mon, Mar 25, 2024 at 2:51 PM Justine Olshan > >> wrote: > >> > I've updated the KIP with the changes I mentioned earlier. I have not > >> yet > >> > removed the --feature-version flag from the upgrade tool. > >> > >> What's the "--feature-version" flag? This is the first time I see it > >> mentioned and I don't see it in the KIP. Did you mean > >> "--release-version"? > >> > >> > Please take a look at the API to get the versions for a given > >> > metadata version. Right now I'm using ApiVersions request and > >> specifying a > >> > metadata version. The supported versions are then supplied in the > >> > ApiVersions response. > >> > There were tradeoffs with this approach. It is a pretty minimal > change, > >> but > >> > reusing the API means that it could be confusing (ie, the ApiKeys will > >> be > >> > supplied in the response but not needed.) I considered making a whole > >> new > >> > API, but that didn't seem necessary for this use. > >> > >> I agree that this is extremely confusing and we shouldn't overload the > >> ApiVersions RPC to return this information. The KIP doesn't mention > >> how it is going to use this API. Do you need to update the Admin > >> client to include this information? > >> > >> Having said this, as you mentioned in the KIP the kafka-storage tool > >> needs this information and that tool cannot assume that there is a > >> running server it can query (send an RPC). Can the kafka-features use > >> the same mechanism used by kafka-storage without calling into a > >> broker? > >> > >> re: "It will work just like the storage tool and upgrade all the > >> features to a version" > >> > >> Does this mean that --release-version cannot be used with > >> "kafka-features downgrade"? > >> > >> re: Consistency with KIP-853 > >> > >> Jun and I have been having a similar conversation in the discussion > >> thread for KIP-853. From what I can tell both changes are compatible. > >> Do you mind taking a look at these two sections and confirming that > >> they
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
I've updated the KIP to include this CLI. For now, I've moved the new api to the server as a rejected alternative. It seems like we can keep the mapping in the tool. Given that we also need to do the same validation for using multiple --feature flags (the request will look the same to the server), we can have the --release-version flag. I think that closes the main discussions. Please let me know if there is any further discussion I missed. Justine On Mon, Mar 25, 2024 at 4:44 PM Justine Olshan wrote: > Hi Jose, > > Sorry for the typos. I think you figured out what I meant. > > I can make a new API. There is a risk of creating a ton of very similar > APIs though. Even the ApiVersions api is confusing with its supported and > finalized features fields. I wonder if there is a middle ground here. > I can have the storage tool and features tool rely on the feature and not > query the server. Colin seemed to be against that. > > Anyway your idea of putting the info on the server side is probably for > the best. > > --release-version can work with the downgrade tool too. I just didn't > think I needed to directly spell that out. I can though. > > I wish we weren't splitting this conversation among the two threads. :( I > tried to get this out so it could cover all KIPs. Having this on separate > threads makes getting this consensus even harder than it already is. > From what I can tell your KIP's text matches this. Is the expectation that > the added flags will be done as part of this KIP or your KIP? I don't > really have a strong opinion about --release-version so maybe it should > have been part of your KIP all along. > > > To me version 0 doesn't necessarily mean that the feature is not > enabled. For example, for kraft.version, version 0 means the protocol > prior to KIP-853. Version 0 is the currently implemented version of > the KRaft protocol. > > This is what Colin told me in our previous discussions. I don't really > feel too strongly about the semantics here. > > So it seems like the only real undecided item here is whether we should > have this new api query the server or rely on the information being built > in the tool. > I will update the KIP to include the CLI command to get the information. > > Justine > > On Mon, Mar 25, 2024 at 4:19 PM José Armando García Sancio > wrote: > >> Hi Justine, >> >> Thanks for the update. See my comments below. >> >> On Mon, Mar 25, 2024 at 2:51 PM Justine Olshan >> wrote: >> > I've updated the KIP with the changes I mentioned earlier. I have not >> yet >> > removed the --feature-version flag from the upgrade tool. >> >> What's the "--feature-version" flag? This is the first time I see it >> mentioned and I don't see it in the KIP. Did you mean >> "--release-version"? >> >> > Please take a look at the API to get the versions for a given >> > metadata version. Right now I'm using ApiVersions request and >> specifying a >> > metadata version. The supported versions are then supplied in the >> > ApiVersions response. >> > There were tradeoffs with this approach. It is a pretty minimal change, >> but >> > reusing the API means that it could be confusing (ie, the ApiKeys will >> be >> > supplied in the response but not needed.) I considered making a whole >> new >> > API, but that didn't seem necessary for this use. >> >> I agree that this is extremely confusing and we shouldn't overload the >> ApiVersions RPC to return this information. The KIP doesn't mention >> how it is going to use this API. Do you need to update the Admin >> client to include this information? >> >> Having said this, as you mentioned in the KIP the kafka-storage tool >> needs this information and that tool cannot assume that there is a >> running server it can query (send an RPC). Can the kafka-features use >> the same mechanism used by kafka-storage without calling into a >> broker? >> >> re: "It will work just like the storage tool and upgrade all the >> features to a version" >> >> Does this mean that --release-version cannot be used with >> "kafka-features downgrade"? >> >> re: Consistency with KIP-853 >> >> Jun and I have been having a similar conversation in the discussion >> thread for KIP-853. From what I can tell both changes are compatible. >> Do you mind taking a look at these two sections and confirming that >> they don't contradict your KIP? >> 1. >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-853%3A+KRaft+Controller+Membership+Changes#KIP853:KRaftControllerMembershipChanges-kafka-storage >> 2. >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-853%3A+KRaft+Controller+Membership+Changes#KIP853:KRaftControllerMembershipChanges-kafka-features >> >> re: nit "For MVs that existed before these features, we map the new >> features to version 0 (no feature enabled)." >> >> To me version 0 doesn't necessarily mean that the feature is not >> enabled. For example, for kraft.version, version 0 means the protocol >> prior to KIP-853. Version 0 is the currently implemented
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi Jose, Sorry for the typos. I think you figured out what I meant. I can make a new API. There is a risk of creating a ton of very similar APIs though. Even the ApiVersions api is confusing with its supported and finalized features fields. I wonder if there is a middle ground here. I can have the storage tool and features tool rely on the feature and not query the server. Colin seemed to be against that. > Anyway your idea of putting the info on the server side is probably for the best. --release-version can work with the downgrade tool too. I just didn't think I needed to directly spell that out. I can though. I wish we weren't splitting this conversation among the two threads. :( I tried to get this out so it could cover all KIPs. Having this on separate threads makes getting this consensus even harder than it already is. >From what I can tell your KIP's text matches this. Is the expectation that the added flags will be done as part of this KIP or your KIP? I don't really have a strong opinion about --release-version so maybe it should have been part of your KIP all along. > To me version 0 doesn't necessarily mean that the feature is not enabled. For example, for kraft.version, version 0 means the protocol prior to KIP-853. Version 0 is the currently implemented version of the KRaft protocol. This is what Colin told me in our previous discussions. I don't really feel too strongly about the semantics here. So it seems like the only real undecided item here is whether we should have this new api query the server or rely on the information being built in the tool. I will update the KIP to include the CLI command to get the information. Justine On Mon, Mar 25, 2024 at 4:19 PM José Armando García Sancio wrote: > Hi Justine, > > Thanks for the update. See my comments below. > > On Mon, Mar 25, 2024 at 2:51 PM Justine Olshan > wrote: > > I've updated the KIP with the changes I mentioned earlier. I have not yet > > removed the --feature-version flag from the upgrade tool. > > What's the "--feature-version" flag? This is the first time I see it > mentioned and I don't see it in the KIP. Did you mean > "--release-version"? > > > Please take a look at the API to get the versions for a given > > metadata version. Right now I'm using ApiVersions request and specifying > a > > metadata version. The supported versions are then supplied in the > > ApiVersions response. > > There were tradeoffs with this approach. It is a pretty minimal change, > but > > reusing the API means that it could be confusing (ie, the ApiKeys will be > > supplied in the response but not needed.) I considered making a whole new > > API, but that didn't seem necessary for this use. > > I agree that this is extremely confusing and we shouldn't overload the > ApiVersions RPC to return this information. The KIP doesn't mention > how it is going to use this API. Do you need to update the Admin > client to include this information? > > Having said this, as you mentioned in the KIP the kafka-storage tool > needs this information and that tool cannot assume that there is a > running server it can query (send an RPC). Can the kafka-features use > the same mechanism used by kafka-storage without calling into a > broker? > > re: "It will work just like the storage tool and upgrade all the > features to a version" > > Does this mean that --release-version cannot be used with > "kafka-features downgrade"? > > re: Consistency with KIP-853 > > Jun and I have been having a similar conversation in the discussion > thread for KIP-853. From what I can tell both changes are compatible. > Do you mind taking a look at these two sections and confirming that > they don't contradict your KIP? > 1. > https://cwiki.apache.org/confluence/display/KAFKA/KIP-853%3A+KRaft+Controller+Membership+Changes#KIP853:KRaftControllerMembershipChanges-kafka-storage > 2. > https://cwiki.apache.org/confluence/display/KAFKA/KIP-853%3A+KRaft+Controller+Membership+Changes#KIP853:KRaftControllerMembershipChanges-kafka-features > > re: nit "For MVs that existed before these features, we map the new > features to version 0 (no feature enabled)." > > To me version 0 doesn't necessarily mean that the feature is not > enabled. For example, for kraft.version, version 0 means the protocol > prior to KIP-853. Version 0 is the currently implemented version of > the KRaft protocol. > > Thanks, > -- > -José >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi Justine, Thanks for the update. See my comments below. On Mon, Mar 25, 2024 at 2:51 PM Justine Olshan wrote: > I've updated the KIP with the changes I mentioned earlier. I have not yet > removed the --feature-version flag from the upgrade tool. What's the "--feature-version" flag? This is the first time I see it mentioned and I don't see it in the KIP. Did you mean "--release-version"? > Please take a look at the API to get the versions for a given > metadata version. Right now I'm using ApiVersions request and specifying a > metadata version. The supported versions are then supplied in the > ApiVersions response. > There were tradeoffs with this approach. It is a pretty minimal change, but > reusing the API means that it could be confusing (ie, the ApiKeys will be > supplied in the response but not needed.) I considered making a whole new > API, but that didn't seem necessary for this use. I agree that this is extremely confusing and we shouldn't overload the ApiVersions RPC to return this information. The KIP doesn't mention how it is going to use this API. Do you need to update the Admin client to include this information? Having said this, as you mentioned in the KIP the kafka-storage tool needs this information and that tool cannot assume that there is a running server it can query (send an RPC). Can the kafka-features use the same mechanism used by kafka-storage without calling into a broker? re: "It will work just like the storage tool and upgrade all the features to a version" Does this mean that --release-version cannot be used with "kafka-features downgrade"? re: Consistency with KIP-853 Jun and I have been having a similar conversation in the discussion thread for KIP-853. From what I can tell both changes are compatible. Do you mind taking a look at these two sections and confirming that they don't contradict your KIP? 1. https://cwiki.apache.org/confluence/display/KAFKA/KIP-853%3A+KRaft+Controller+Membership+Changes#KIP853:KRaftControllerMembershipChanges-kafka-storage 2. https://cwiki.apache.org/confluence/display/KAFKA/KIP-853%3A+KRaft+Controller+Membership+Changes#KIP853:KRaftControllerMembershipChanges-kafka-features re: nit "For MVs that existed before these features, we map the new features to version 0 (no feature enabled)." To me version 0 doesn't necessarily mean that the feature is not enabled. For example, for kraft.version, version 0 means the protocol prior to KIP-853. Version 0 is the currently implemented version of the KRaft protocol. Thanks, -- -José
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hey all, I've updated the KIP with the changes I mentioned earlier. I have not yet removed the --feature-version flag from the upgrade tool. Please take a look at the API to get the versions for a given metadata version. Right now I'm using ApiVersions request and specifying a metadata version. The supported versions are then supplied in the ApiVersions response. There were tradeoffs with this approach. It is a pretty minimal change, but reusing the API means that it could be confusing (ie, the ApiKeys will be supplied in the response but not needed.) I considered making a whole new API, but that didn't seem necessary for this use. Please please try to let me know in the next few days, I hope to start a vote soon. Thanks, Justine On Mon, Mar 18, 2024 at 9:17 AM Justine Olshan wrote: > Hey folks, > > I didn't have a strong preference for requesting the versions via the tool > only or getting it from the server. Colin seemed to suggest it was "for the > best" to request from the server to make the tool lightweight. > I guess the argument against that is having to build and support another > API. > > It also seems like there may be some confusion -- partially on my part. > For the tools, I had a question about the feature upgrade tool. So it seems > like we already support multiple features via the `--feature` flag, we > simply rely on the server side to throw errors now? > > Justine > > On Fri, Mar 15, 2024 at 3:38 PM José Armando García Sancio > wrote: > >> Hi Justine, >> >> Thanks for the update. Some comments below. >> >> On Wed, Mar 13, 2024 at 10:53 AM Justine Olshan >> wrote: >> > 4. Include an API to list the features for a given metadata version >> >> I am not opposed to designing and implementing this. I am just >> wondering if this is strictly required? >> >> Would having auto-generated documentation address the issue of not >> knowing which feature versions are associated with a given release >> version? >> >> Does it need to be a Kafka API (RPC)? Or can this be strictly >> implemented in the tool? The latest tool, which is needed to parse >> release version to feature version, can print this mapping. Is this >> what you mean by API? >> >> > 5. I'm going back and forth on whether we should support the >> > --release-version flag still. If we do, we need to include validation >> so we >> > only upgrade on upgrade. >> >> I am not sure how this is different from supporting multiple --feature >> flags. The user can run an upgrade command where some of the features >> specified are greater than what the cluster has finalized and some of >> the features specified are less than what the cluster has finalized. >> >> In other words, the KRaft controller and kafka-storage tool are going >> to have to implement this validation even if you don't implement >> --release-version in the tools. >> Thanks, >> -- >> -José >> >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hey folks, I didn't have a strong preference for requesting the versions via the tool only or getting it from the server. Colin seemed to suggest it was "for the best" to request from the server to make the tool lightweight. I guess the argument against that is having to build and support another API. It also seems like there may be some confusion -- partially on my part. For the tools, I had a question about the feature upgrade tool. So it seems like we already support multiple features via the `--feature` flag, we simply rely on the server side to throw errors now? Justine On Fri, Mar 15, 2024 at 3:38 PM José Armando García Sancio wrote: > Hi Justine, > > Thanks for the update. Some comments below. > > On Wed, Mar 13, 2024 at 10:53 AM Justine Olshan > wrote: > > 4. Include an API to list the features for a given metadata version > > I am not opposed to designing and implementing this. I am just > wondering if this is strictly required? > > Would having auto-generated documentation address the issue of not > knowing which feature versions are associated with a given release > version? > > Does it need to be a Kafka API (RPC)? Or can this be strictly > implemented in the tool? The latest tool, which is needed to parse > release version to feature version, can print this mapping. Is this > what you mean by API? > > > 5. I'm going back and forth on whether we should support the > > --release-version flag still. If we do, we need to include validation so > we > > only upgrade on upgrade. > > I am not sure how this is different from supporting multiple --feature > flags. The user can run an upgrade command where some of the features > specified are greater than what the cluster has finalized and some of > the features specified are less than what the cluster has finalized. > > In other words, the KRaft controller and kafka-storage tool are going > to have to implement this validation even if you don't implement > --release-version in the tools. > Thanks, > -- > -José >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi Justine, Thanks for the update. Some comments below. On Wed, Mar 13, 2024 at 10:53 AM Justine Olshan wrote: > 4. Include an API to list the features for a given metadata version I am not opposed to designing and implementing this. I am just wondering if this is strictly required? Would having auto-generated documentation address the issue of not knowing which feature versions are associated with a given release version? Does it need to be a Kafka API (RPC)? Or can this be strictly implemented in the tool? The latest tool, which is needed to parse release version to feature version, can print this mapping. Is this what you mean by API? > 5. I'm going back and forth on whether we should support the > --release-version flag still. If we do, we need to include validation so we > only upgrade on upgrade. I am not sure how this is different from supporting multiple --feature flags. The user can run an upgrade command where some of the features specified are greater than what the cluster has finalized and some of the features specified are less than what the cluster has finalized. In other words, the KRaft controller and kafka-storage tool are going to have to implement this validation even if you don't implement --release-version in the tools. Thanks, -- -José
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi Justine, Thanks for driving this forward. Comments below. On Wed, Mar 13, 2024, at 10:51, Justine Olshan wrote: > Hey folks -- let me summarize my understanding > > Artem requested we change the name of transaction version (tv) to > transaction protocol version (tpv). If I do that, I will also probably > change to group coordinator protocol version (gcpv). > > > Folks were concerned about upgrades/downgrades with different feature > versions when using the feature tool and --release version. I think we can > address some concerns here: > >- don't allow a command that moves a version in the wrong direction >(when upgrade actually downgrades a feature for example) >- allow a command to describe all the feature versions for a given >metadata version > > Note that Colin expressed some concern with having the --release-version > flag at all. I wonder if we can reach a compromise with having the upgrade > command have a "latest" command only. > > > Jun proposed this "latest" functionality can be the default when no version > or feature is specified. > > Jose requested that we make --release-version and --feature mutually > exclusive for both the storage tool and the features tool. We should also > specify each feature in the storage tool as a separate flag. > > Finally, Jun and Jose requested deprecation of the --metadata flag, but > Colin felt we should keep it. I mentioned the --feature flag does the same > but no reply yet. > > * So here are the updates I propose:* > 1. Update the feature names as Artem described -- transaction protocol > version and group coordinator protocol version > > 2. Rename --features in the storage tool to --feature. In the case where we > use this flag, we must repeat the flag for the number of features to set > all of them. > +1 > 3. No argument behavior on the storage and upgrade tools is to set all the > features to the latest (stable) version > +1 > 4. Include an API to list the features for a given metadata version > At the risk of expanding the scope... if we have an API to list feature levels, we should have a brief description of what they do. Something like: 13 3.6-IV1 add metadata transactions Maybe a simple way to do this would just be to output the full list and let people grep for what they want. You could also add filters like --release-version, etc. The big question is would you store this data on the servers or in the tool? Right now the tool is a bit "smart" in the sense that it can translate between an MV string and an MV level. Arguably it should be dumber, though, and rely on the server for these things. Anyway your idea of putting the info on the server side is probably for the best. > 5. I'm going back and forth on whether we should support the > --release-version flag still. If we do, we need to include validation so we > only upgrade on upgrade. I'd rather not, but I'm curious what others think. I really feel like the use-case is not there (compared with the more manual, but safer process described above) best, Colin > > Let me know if folks have any issues with the updates I proposed or think > there is something I missed. > > Justine > > On Fri, Mar 8, 2024 at 11:59 AM Artem Livshits > wrote: > >> Hi Justine, >> >> > Are you suggesting it should be called "transaction protocol version" or >> "TPV"? I don't mind that, but just wanted to clarify if we want to include >> protocol or if simply "transaction version" is enough. >> >> My understanding is that "metadata version" is the version of metadata >> records, which is fairly straightforward. "Transaction version" may be >> ambiguous. >> >> -Artem >> >> On Thu, Feb 29, 2024 at 3:39 PM Justine Olshan >> >> wrote: >> >> > Hey folks, >> > >> > Thanks for the discussion. Let me try to cover everyone's comments. >> > >> > Artem -- >> > I can add the examples you mentioned. As for naming, right now the >> feature >> > is called "transaction version" or "TV". Are you suggesting it should be >> > called "transaction protocol version" or "TPV"? I don't mind that, but >> just >> > wanted to clarify if we want to include protocol or if simply >> "transaction >> > version" is enough. >> > >> > Jun -- >> > >> > 10. *With **more features, would each of those be controlled by a >> separate >> > feature or* >> > >> > *multiple features. For example, is the new transaction record format* >> > >> > *controlled only by MV with TV having a dependency on MV or is it >> > controlled* >> > >> > *by both MV and TV.* >> > >> > >> > I think this will need to be decided on a case by case basis. There >> should >> > be a mechanism to set dependencies among features. >> > For transaction version specifically, I have no metadata version >> > dependencies besides requiring 3.3 to write the feature records and use >> the >> > feature tools. I would suspect all new features would have this >> > requirement. >> > >> > >> > 11. *Basically, if **--release-version is not used, the command
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hey folks -- let me summarize my understanding Artem requested we change the name of transaction version (tv) to transaction protocol version (tpv). If I do that, I will also probably change to group coordinator protocol version (gcpv). Folks were concerned about upgrades/downgrades with different feature versions when using the feature tool and --release version. I think we can address some concerns here: - don't allow a command that moves a version in the wrong direction (when upgrade actually downgrades a feature for example) - allow a command to describe all the feature versions for a given metadata version Note that Colin expressed some concern with having the --release-version flag at all. I wonder if we can reach a compromise with having the upgrade command have a "latest" command only. Jun proposed this "latest" functionality can be the default when no version or feature is specified. Jose requested that we make --release-version and --feature mutually exclusive for both the storage tool and the features tool. We should also specify each feature in the storage tool as a separate flag. Finally, Jun and Jose requested deprecation of the --metadata flag, but Colin felt we should keep it. I mentioned the --feature flag does the same but no reply yet. * So here are the updates I propose:* 1. Update the feature names as Artem described -- transaction protocol version and group coordinator protocol version 2. Rename --features in the storage tool to --feature. In the case where we use this flag, we must repeat the flag for the number of features to set all of them. 3. No argument behavior on the storage and upgrade tools is to set all the features to the latest (stable) version 4. Include an API to list the features for a given metadata version 5. I'm going back and forth on whether we should support the --release-version flag still. If we do, we need to include validation so we only upgrade on upgrade. Let me know if folks have any issues with the updates I proposed or think there is something I missed. Justine On Fri, Mar 8, 2024 at 11:59 AM Artem Livshits wrote: > Hi Justine, > > > Are you suggesting it should be called "transaction protocol version" or > "TPV"? I don't mind that, but just wanted to clarify if we want to include > protocol or if simply "transaction version" is enough. > > My understanding is that "metadata version" is the version of metadata > records, which is fairly straightforward. "Transaction version" may be > ambiguous. > > -Artem > > On Thu, Feb 29, 2024 at 3:39 PM Justine Olshan > > wrote: > > > Hey folks, > > > > Thanks for the discussion. Let me try to cover everyone's comments. > > > > Artem -- > > I can add the examples you mentioned. As for naming, right now the > feature > > is called "transaction version" or "TV". Are you suggesting it should be > > called "transaction protocol version" or "TPV"? I don't mind that, but > just > > wanted to clarify if we want to include protocol or if simply > "transaction > > version" is enough. > > > > Jun -- > > > > 10. *With **more features, would each of those be controlled by a > separate > > feature or* > > > > *multiple features. For example, is the new transaction record format* > > > > *controlled only by MV with TV having a dependency on MV or is it > > controlled* > > > > *by both MV and TV.* > > > > > > I think this will need to be decided on a case by case basis. There > should > > be a mechanism to set dependencies among features. > > For transaction version specifically, I have no metadata version > > dependencies besides requiring 3.3 to write the feature records and use > the > > feature tools. I would suspect all new features would have this > > requirement. > > > > > > 11. *Basically, if **--release-version is not used, the command will just > > use the latest* > > > > *production version of every feature. Should we apply that logic to both* > > > > *tools?* > > > > > > How would this work with the upgrade tool? I think we want a way to set a > > new feature version for one feature and not touch any of the others. > > > > > > *12. Should we remove --metadata METADATA from kafka-features? It does > the* > > > > *same thing as --release-version.* > > > > > > When I previously discussed with Colin McCabe offline about this tool, he > > was strongly against deprecation or changing flags. I personally think it > > could good > > > > to unify and not support a ton of flags, but I would want to make sure he > > is aligned. > > > > > > *13. KIP-853 also extends the tools to support a new feature > > kraft.version.* > > > > *It would be useful to have alignment between that KIP and this one.* > > > > > > Sounds good. Looks like Jose is in on the discussion so we can continue > > here. :) > > > > > > > > Jose -- > > > > > > *1. KIP-853 uses --feature for kafka-storage instead of --features.* > > > > *This is consistent with the use of --feature in the "kafka-feature.sh* > > > > *upgrade"
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi Justine, > Are you suggesting it should be called "transaction protocol version" or "TPV"? I don't mind that, but just wanted to clarify if we want to include protocol or if simply "transaction version" is enough. My understanding is that "metadata version" is the version of metadata records, which is fairly straightforward. "Transaction version" may be ambiguous. -Artem On Thu, Feb 29, 2024 at 3:39 PM Justine Olshan wrote: > Hey folks, > > Thanks for the discussion. Let me try to cover everyone's comments. > > Artem -- > I can add the examples you mentioned. As for naming, right now the feature > is called "transaction version" or "TV". Are you suggesting it should be > called "transaction protocol version" or "TPV"? I don't mind that, but just > wanted to clarify if we want to include protocol or if simply "transaction > version" is enough. > > Jun -- > > 10. *With **more features, would each of those be controlled by a separate > feature or* > > *multiple features. For example, is the new transaction record format* > > *controlled only by MV with TV having a dependency on MV or is it > controlled* > > *by both MV and TV.* > > > I think this will need to be decided on a case by case basis. There should > be a mechanism to set dependencies among features. > For transaction version specifically, I have no metadata version > dependencies besides requiring 3.3 to write the feature records and use the > feature tools. I would suspect all new features would have this > requirement. > > > 11. *Basically, if **--release-version is not used, the command will just > use the latest* > > *production version of every feature. Should we apply that logic to both* > > *tools?* > > > How would this work with the upgrade tool? I think we want a way to set a > new feature version for one feature and not touch any of the others. > > > *12. Should we remove --metadata METADATA from kafka-features? It does the* > > *same thing as --release-version.* > > > When I previously discussed with Colin McCabe offline about this tool, he > was strongly against deprecation or changing flags. I personally think it > could good > > to unify and not support a ton of flags, but I would want to make sure he > is aligned. > > > *13. KIP-853 also extends the tools to support a new feature > kraft.version.* > > *It would be useful to have alignment between that KIP and this one.* > > > Sounds good. Looks like Jose is in on the discussion so we can continue > here. :) > > > > Jose -- > > > *1. KIP-853 uses --feature for kafka-storage instead of --features.* > > *This is consistent with the use of --feature in the "kafka-feature.sh* > > *upgrade" command.* > > > I wanted to include multiple features in one command, so it seems like > features is a better name. I discuss more below about why I think we should > allow setting multiple features at once. > > > *2. I find it a bit inconsistent that --feature and --release-version* > > *are mutually exclusive in the kafka-feature CLI but not in the* > > *kafka-storage CLI. What is the reason for this decision?* > > > For the storage tool, we are setting all the features for the cluster. By > default, all are set. For the upgrade tool, the default is to set one > feature. In the storage tool, it is natural for the --release-version to > set the remaining features that --features didn't cover since otherwise we > would need to set them all > > If we use the flag. In the feature upgrade case, it is less necessary for > all the features to be set at once and the tool can be run multiple times. > I'm not opposed to allowing both flags, but it is less necessary in my > opinion. > > > *3. KIP-853 deprecates --metadata in the kafka-features and makes it an* > > *alias for --release-version. In KIP-1022, what happens if the user* > > *specified both --metadata and --feature?* > > > See my note above (Jun's comment 12) about deprecating old commands. I > would think as the KIP stands now, we would not accept both commands. > > > *4. I would suggest keeping this* > > *consistent with kafka-features. It would avoid having to implement one* > > *more parser in Kafka.* > > > I sort of already implemented it as such, so I don't think it is too > tricky. I'm not sure of an alternative. Kafka features currently only > supports one feature at a time. > I would like to support more than one for the storage tool. Do you have > another suggestion for multiple features in the storage tool? > > > *5. As currently described, trial and error seem to be the* > > *only mechanism. Should the Kafka documentation describe these* > > *dependencies? Is that good enough?* > > > The plan so far is documentation. The idea is that this is an advanced > feature, so I think it is reasonable to ask folks use documentation > > > *6. Did you mean that 3.8-IV4 would map to TV2? If* > > *not, 3.8-IV3 would map to two different TV values.* > > > It was a typo. Each MV maps to a single other feature version. > > > *7. For
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi Colin, Thanks for bringing up these points. I believe Jose suggested failing the command if some features upgrade on the downgrade call and vice versa. This could address some of the concerns you bring up here. I agree we should not have an rpc to do both. As for setting metadata only, we do have the option of using the --feature flag to set only metadata version, but that requires writing it by short rather than by metadata string. Not sure if that's what folks had in mind. Including a command to list the features for a given version is pretty useful. I can add that to the kip unless folks have concerns. Justine On Thu, Mar 7, 2024 at 1:28 PM Colin McCabe wrote: > Hi Jun and Justine, > > My understanding is that in the KIP, --metadata does not do the same thing > as --release-version. The --metadata flag sets the metadata version only, > and does not change any other feature. So neither flag replaces the other. > > In general, I'm not sure that this functionality belongs in the features > command. It will have a lot of confusing interactions with the other flags. > If we do want it in the features command it should be its own subcommand, > not upgrade or downgrade. Because setting all the features could involve > upgrading some, but downgrading others. So really what we're doing is > neither an upgrade nor a downgrade, but a sync to a release version. > > This also raises some questions about the RPC. If we want to use the > existing features RPC, we cannot perform a sync that does both upgrades and > downgrades in a single RPC. So we'd either need to be non-atomic, or just > refuse to do it in that case. Or add a new server-side RPC for this. > > Perhaps it would be more useful to just have a command that tells people > what the levels of each feature would be for a given release version? Then > the user could choose which ones to upgrade / downgrade (if any) and do > them carefully one at a time. The shotgun approach, where you upgrade > everything at once, actually isn't what I think most users want to do in > production. > > best, > Colin > > > On Fri, Mar 1, 2024, at 13:25, Jun Rao wrote: > > Hi, Justine, > > > > Thanks for the reply. > > > > 11. Yes, we can still have the option to set individual features. I was > > suggesting that if one uses the tool without either --release-version > > or --features, it will just use the latest version of every feature that > it > > knows. This is what's proposed in the original KIP-584. > > > > 12. If we can't deprecate --metadata METADATA, it would be useful to > > describe the reason. > > > > Jun > > > > On Thu, Feb 29, 2024 at 3:39 PM Justine Olshan > > > wrote: > > > >> Hey folks, > >> > >> Thanks for the discussion. Let me try to cover everyone's comments. > >> > >> Artem -- > >> I can add the examples you mentioned. As for naming, right now the > feature > >> is called "transaction version" or "TV". Are you suggesting it should be > >> called "transaction protocol version" or "TPV"? I don't mind that, but > just > >> wanted to clarify if we want to include protocol or if simply > "transaction > >> version" is enough. > >> > >> Jun -- > >> > >> 10. *With **more features, would each of those be controlled by a > separate > >> feature or* > >> > >> *multiple features. For example, is the new transaction record format* > >> > >> *controlled only by MV with TV having a dependency on MV or is it > >> controlled* > >> > >> *by both MV and TV.* > >> > >> > >> I think this will need to be decided on a case by case basis. There > should > >> be a mechanism to set dependencies among features. > >> For transaction version specifically, I have no metadata version > >> dependencies besides requiring 3.3 to write the feature records and use > the > >> feature tools. I would suspect all new features would have this > >> requirement. > >> > >> > >> 11. *Basically, if **--release-version is not used, the command will > just > >> use the latest* > >> > >> *production version of every feature. Should we apply that logic to > both* > >> > >> *tools?* > >> > >> > >> How would this work with the upgrade tool? I think we want a way to set > a > >> new feature version for one feature and not touch any of the others. > >> > >> > >> *12. Should we remove --metadata METADATA from kafka-features? It does > the* > >> > >> *same thing as --release-version.* > >> > >> > >> When I previously discussed with Colin McCabe offline about this tool, > he > >> was strongly against deprecation or changing flags. I personally think > it > >> could good > >> > >> to unify and not support a ton of flags, but I would want to make sure > he > >> is aligned. > >> > >> > >> *13. KIP-853 also extends the tools to support a new feature > >> kraft.version.* > >> > >> *It would be useful to have alignment between that KIP and this one.* > >> > >> > >> Sounds good. Looks like Jose is in on the discussion so we can continue > >> here. :) > >> > >> > >> > >> Jose -- > >> > >> > >> *1. KIP-853 uses
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi Jun and Justine, My understanding is that in the KIP, --metadata does not do the same thing as --release-version. The --metadata flag sets the metadata version only, and does not change any other feature. So neither flag replaces the other. In general, I'm not sure that this functionality belongs in the features command. It will have a lot of confusing interactions with the other flags. If we do want it in the features command it should be its own subcommand, not upgrade or downgrade. Because setting all the features could involve upgrading some, but downgrading others. So really what we're doing is neither an upgrade nor a downgrade, but a sync to a release version. This also raises some questions about the RPC. If we want to use the existing features RPC, we cannot perform a sync that does both upgrades and downgrades in a single RPC. So we'd either need to be non-atomic, or just refuse to do it in that case. Or add a new server-side RPC for this. Perhaps it would be more useful to just have a command that tells people what the levels of each feature would be for a given release version? Then the user could choose which ones to upgrade / downgrade (if any) and do them carefully one at a time. The shotgun approach, where you upgrade everything at once, actually isn't what I think most users want to do in production. best, Colin On Fri, Mar 1, 2024, at 13:25, Jun Rao wrote: > Hi, Justine, > > Thanks for the reply. > > 11. Yes, we can still have the option to set individual features. I was > suggesting that if one uses the tool without either --release-version > or --features, it will just use the latest version of every feature that it > knows. This is what's proposed in the original KIP-584. > > 12. If we can't deprecate --metadata METADATA, it would be useful to > describe the reason. > > Jun > > On Thu, Feb 29, 2024 at 3:39 PM Justine Olshan > wrote: > >> Hey folks, >> >> Thanks for the discussion. Let me try to cover everyone's comments. >> >> Artem -- >> I can add the examples you mentioned. As for naming, right now the feature >> is called "transaction version" or "TV". Are you suggesting it should be >> called "transaction protocol version" or "TPV"? I don't mind that, but just >> wanted to clarify if we want to include protocol or if simply "transaction >> version" is enough. >> >> Jun -- >> >> 10. *With **more features, would each of those be controlled by a separate >> feature or* >> >> *multiple features. For example, is the new transaction record format* >> >> *controlled only by MV with TV having a dependency on MV or is it >> controlled* >> >> *by both MV and TV.* >> >> >> I think this will need to be decided on a case by case basis. There should >> be a mechanism to set dependencies among features. >> For transaction version specifically, I have no metadata version >> dependencies besides requiring 3.3 to write the feature records and use the >> feature tools. I would suspect all new features would have this >> requirement. >> >> >> 11. *Basically, if **--release-version is not used, the command will just >> use the latest* >> >> *production version of every feature. Should we apply that logic to both* >> >> *tools?* >> >> >> How would this work with the upgrade tool? I think we want a way to set a >> new feature version for one feature and not touch any of the others. >> >> >> *12. Should we remove --metadata METADATA from kafka-features? It does the* >> >> *same thing as --release-version.* >> >> >> When I previously discussed with Colin McCabe offline about this tool, he >> was strongly against deprecation or changing flags. I personally think it >> could good >> >> to unify and not support a ton of flags, but I would want to make sure he >> is aligned. >> >> >> *13. KIP-853 also extends the tools to support a new feature >> kraft.version.* >> >> *It would be useful to have alignment between that KIP and this one.* >> >> >> Sounds good. Looks like Jose is in on the discussion so we can continue >> here. :) >> >> >> >> Jose -- >> >> >> *1. KIP-853 uses --feature for kafka-storage instead of --features.* >> >> *This is consistent with the use of --feature in the "kafka-feature.sh* >> >> *upgrade" command.* >> >> >> I wanted to include multiple features in one command, so it seems like >> features is a better name. I discuss more below about why I think we should >> allow setting multiple features at once. >> >> >> *2. I find it a bit inconsistent that --feature and --release-version* >> >> *are mutually exclusive in the kafka-feature CLI but not in the* >> >> *kafka-storage CLI. What is the reason for this decision?* >> >> >> For the storage tool, we are setting all the features for the cluster. By >> default, all are set. For the upgrade tool, the default is to set one >> feature. In the storage tool, it is natural for the --release-version to >> set the remaining features that --features didn't cover since otherwise we >> would need to set them all >> >> If we
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi, Justine, Thanks for the reply. 11. Yes, we can still have the option to set individual features. I was suggesting that if one uses the tool without either --release-version or --features, it will just use the latest version of every feature that it knows. This is what's proposed in the original KIP-584. 12. If we can't deprecate --metadata METADATA, it would be useful to describe the reason. Jun On Thu, Feb 29, 2024 at 3:39 PM Justine Olshan wrote: > Hey folks, > > Thanks for the discussion. Let me try to cover everyone's comments. > > Artem -- > I can add the examples you mentioned. As for naming, right now the feature > is called "transaction version" or "TV". Are you suggesting it should be > called "transaction protocol version" or "TPV"? I don't mind that, but just > wanted to clarify if we want to include protocol or if simply "transaction > version" is enough. > > Jun -- > > 10. *With **more features, would each of those be controlled by a separate > feature or* > > *multiple features. For example, is the new transaction record format* > > *controlled only by MV with TV having a dependency on MV or is it > controlled* > > *by both MV and TV.* > > > I think this will need to be decided on a case by case basis. There should > be a mechanism to set dependencies among features. > For transaction version specifically, I have no metadata version > dependencies besides requiring 3.3 to write the feature records and use the > feature tools. I would suspect all new features would have this > requirement. > > > 11. *Basically, if **--release-version is not used, the command will just > use the latest* > > *production version of every feature. Should we apply that logic to both* > > *tools?* > > > How would this work with the upgrade tool? I think we want a way to set a > new feature version for one feature and not touch any of the others. > > > *12. Should we remove --metadata METADATA from kafka-features? It does the* > > *same thing as --release-version.* > > > When I previously discussed with Colin McCabe offline about this tool, he > was strongly against deprecation or changing flags. I personally think it > could good > > to unify and not support a ton of flags, but I would want to make sure he > is aligned. > > > *13. KIP-853 also extends the tools to support a new feature > kraft.version.* > > *It would be useful to have alignment between that KIP and this one.* > > > Sounds good. Looks like Jose is in on the discussion so we can continue > here. :) > > > > Jose -- > > > *1. KIP-853 uses --feature for kafka-storage instead of --features.* > > *This is consistent with the use of --feature in the "kafka-feature.sh* > > *upgrade" command.* > > > I wanted to include multiple features in one command, so it seems like > features is a better name. I discuss more below about why I think we should > allow setting multiple features at once. > > > *2. I find it a bit inconsistent that --feature and --release-version* > > *are mutually exclusive in the kafka-feature CLI but not in the* > > *kafka-storage CLI. What is the reason for this decision?* > > > For the storage tool, we are setting all the features for the cluster. By > default, all are set. For the upgrade tool, the default is to set one > feature. In the storage tool, it is natural for the --release-version to > set the remaining features that --features didn't cover since otherwise we > would need to set them all > > If we use the flag. In the feature upgrade case, it is less necessary for > all the features to be set at once and the tool can be run multiple times. > I'm not opposed to allowing both flags, but it is less necessary in my > opinion. > > > *3. KIP-853 deprecates --metadata in the kafka-features and makes it an* > > *alias for --release-version. In KIP-1022, what happens if the user* > > *specified both --metadata and --feature?* > > > See my note above (Jun's comment 12) about deprecating old commands. I > would think as the KIP stands now, we would not accept both commands. > > > *4. I would suggest keeping this* > > *consistent with kafka-features. It would avoid having to implement one* > > *more parser in Kafka.* > > > I sort of already implemented it as such, so I don't think it is too > tricky. I'm not sure of an alternative. Kafka features currently only > supports one feature at a time. > I would like to support more than one for the storage tool. Do you have > another suggestion for multiple features in the storage tool? > > > *5. As currently described, trial and error seem to be the* > > *only mechanism. Should the Kafka documentation describe these* > > *dependencies? Is that good enough?* > > > The plan so far is documentation. The idea is that this is an advanced > feature, so I think it is reasonable to ask folks use documentation > > > *6. Did you mean that 3.8-IV4 would map to TV2? If* > > *not, 3.8-IV3 would map to two different TV values.* > > > It was a typo. Each MV maps to a single other feature version.
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Thanks José -- Let me answer a few quick things If you are not going to deprecate or alias --metadata what happens if the user uses these flags in one command: "kafka-features upgrade --metadata 3.8 --feature kraft.version=1" > I would think as the KIP stands now, we would not accept both commands Metadata flag was added as part of KIP-778. See https://cwiki.apache.org/confluence/display/KAFKA/KIP-778%3A+KRaft+to+KRaft+Upgrades and https://github.com/apache/kafka/commit/28d5a059438634db6fdecdbb816e2584715884d6 I think the usage of --feature multiple times is a little bit clunky, but we can use it for consistency with the existing tool. I was hoping it could be simple to set one feature differently and take the default for everything else. But alas. For the storage tool, if we have to set all the features, do you expect to throw an error when one is missing? This seems acceptable, but just wanted to clarify. For documentation, we can have an autogenerated doc. I haven't thought too much how this would look yet. I am ok with throwing an error if a feature isn't moving in the correct direction (upgrade or downgrade) for the command that is given. I will continue to think about everyone's comments and update the KIP over the next few days. Thanks, Justine On Thu, Feb 29, 2024 at 4:31 PM José Armando García Sancio wrote: > Thanks for the reply Justine. See my comments below: > > On Thu, Feb 29, 2024 at 3:39 PM Justine Olshan > wrote: > > I wanted to include multiple features in one command, so it seems like > > features is a better name. I discuss more below about why I think we > should > > allow setting multiple features at once. > > --feature in kafka-features allows for multiple features to be > specified. Please see the implementation of the tool and > UpdateFeatures RPC. > > For example, you can execute this command kafka-features.sh upgrade > --feature metadata.version=15 --feature kraft.version=1. You should be > able to support the same UI in kafka-storage.sh. It is what KIP-853 > suggests in the interest of making it consistent across CLI. > > > For the storage tool, we are setting all the features for the cluster. By > > default, all are set. For the upgrade tool, the default is to set one > > feature. In the storage tool, it is natural for the --release-version to > > set the remaining features that --features didn't cover since otherwise > we > > would need to set them all > > > > If we use the flag. In the feature upgrade case, it is less necessary for > > all the features to be set at once and the tool can be run multiple > times. > > I'm not opposed to allowing both flags, but it is less necessary in my > > opinion. > > I was thinking of making them mutually exclusive in both cases. Much > easier to explain and document. If the user wants to use --feature in > kafka-storage, they need to specify all of the supported features. > > For the "kafka-feature upgrade" case, they can enumerate all of the > --feature versions that they want to upgrade in one command. > > > See my note above (Jun's comment 12) about deprecating old commands. I > > would think as the KIP stands now, we would not accept both commands. > > If you are not going to deprecate or alias --metadata what happens if > the user uses these flags in one command: "kafka-features upgrade > --metadata 3.8 --feature kraft.version=1" > > One of the problems I am having is that I can't seem to find the KIP > that introduces --metadata so I can only speculate as to the intent of > this flag from the CLI help and implementation. Do you know which KIP > added that flag? > > > I sort of already implemented it as such, so I don't think it is too > > tricky. I'm not sure of an alternative. Kafka features currently only > > supports one feature at a time. > > I would like to support more than one for the storage tool. Do you have > > another suggestion for multiple features in the storage tool? > > "kafka-features.sh upgrade" supports multiple --feature flags. Please > see the tools implementation and the UpdateFeatures RPC. > > You can specify multiple features in the storage tool with: > "kafka-storage format --feature kraft.version=1 --feature > metadata.version=15". The command line parser used by both tools > support flags that append values to a list. This is how the > kafka-features CLI works already. > > > The plan so far is documentation. The idea is that this is an advanced > > feature, so I think it is reasonable to ask folks use documentation > > Are you going to generate the documentation of these dependencies > automatically from the released code like we do for Kafka > configuration properties? Or do Kafka developers need to remember to > update the documentation with these dependencies? > > > The idea I had was that the cluster will check if all the versions are > > supported. If any version is not supported the tool will throw an error. > We > > can also issue a warning if the given command (if supported by the >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Thanks for the reply Justine. See my comments below: On Thu, Feb 29, 2024 at 3:39 PM Justine Olshan wrote: > I wanted to include multiple features in one command, so it seems like > features is a better name. I discuss more below about why I think we should > allow setting multiple features at once. --feature in kafka-features allows for multiple features to be specified. Please see the implementation of the tool and UpdateFeatures RPC. For example, you can execute this command kafka-features.sh upgrade --feature metadata.version=15 --feature kraft.version=1. You should be able to support the same UI in kafka-storage.sh. It is what KIP-853 suggests in the interest of making it consistent across CLI. > For the storage tool, we are setting all the features for the cluster. By > default, all are set. For the upgrade tool, the default is to set one > feature. In the storage tool, it is natural for the --release-version to > set the remaining features that --features didn't cover since otherwise we > would need to set them all > > If we use the flag. In the feature upgrade case, it is less necessary for > all the features to be set at once and the tool can be run multiple times. > I'm not opposed to allowing both flags, but it is less necessary in my > opinion. I was thinking of making them mutually exclusive in both cases. Much easier to explain and document. If the user wants to use --feature in kafka-storage, they need to specify all of the supported features. For the "kafka-feature upgrade" case, they can enumerate all of the --feature versions that they want to upgrade in one command. > See my note above (Jun's comment 12) about deprecating old commands. I > would think as the KIP stands now, we would not accept both commands. If you are not going to deprecate or alias --metadata what happens if the user uses these flags in one command: "kafka-features upgrade --metadata 3.8 --feature kraft.version=1" One of the problems I am having is that I can't seem to find the KIP that introduces --metadata so I can only speculate as to the intent of this flag from the CLI help and implementation. Do you know which KIP added that flag? > I sort of already implemented it as such, so I don't think it is too > tricky. I'm not sure of an alternative. Kafka features currently only > supports one feature at a time. > I would like to support more than one for the storage tool. Do you have > another suggestion for multiple features in the storage tool? "kafka-features.sh upgrade" supports multiple --feature flags. Please see the tools implementation and the UpdateFeatures RPC. You can specify multiple features in the storage tool with: "kafka-storage format --feature kraft.version=1 --feature metadata.version=15". The command line parser used by both tools support flags that append values to a list. This is how the kafka-features CLI works already. > The plan so far is documentation. The idea is that this is an advanced > feature, so I think it is reasonable to ask folks use documentation Are you going to generate the documentation of these dependencies automatically from the released code like we do for Kafka configuration properties? Or do Kafka developers need to remember to update the documentation with these dependencies? > The idea I had was that the cluster will check if all the versions are > supported. If any version is not supported the tool will throw an error. We > can also issue a warning if the given command (if supported by the cluster) > will downgrade a feature. One option is also to require a yes/no prompt or > flag to allow downgrades. The downgrade tool would allow the same. > Alternatively we could just fail the command if a feature is downgraded on > upgrade command or vice versa. I don't have a strong preference. "kafka-features upgrade" should only upgrade and "kafka-features downgrade" command should only downgrade. It should be an error if any of the UpdateFeatures requests violates this. What we need to do is define if an error in one feature results in an error for the entire request. The UpdateFeatures RPC seems to allow individual errors but that gets confusing and difficult to enforce once you introduce dependencies between feature versions. Thanks! -- -José
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hey folks, Thanks for the discussion. Let me try to cover everyone's comments. Artem -- I can add the examples you mentioned. As for naming, right now the feature is called "transaction version" or "TV". Are you suggesting it should be called "transaction protocol version" or "TPV"? I don't mind that, but just wanted to clarify if we want to include protocol or if simply "transaction version" is enough. Jun -- 10. *With **more features, would each of those be controlled by a separate feature or* *multiple features. For example, is the new transaction record format* *controlled only by MV with TV having a dependency on MV or is it controlled* *by both MV and TV.* I think this will need to be decided on a case by case basis. There should be a mechanism to set dependencies among features. For transaction version specifically, I have no metadata version dependencies besides requiring 3.3 to write the feature records and use the feature tools. I would suspect all new features would have this requirement. 11. *Basically, if **--release-version is not used, the command will just use the latest* *production version of every feature. Should we apply that logic to both* *tools?* How would this work with the upgrade tool? I think we want a way to set a new feature version for one feature and not touch any of the others. *12. Should we remove --metadata METADATA from kafka-features? It does the* *same thing as --release-version.* When I previously discussed with Colin McCabe offline about this tool, he was strongly against deprecation or changing flags. I personally think it could good to unify and not support a ton of flags, but I would want to make sure he is aligned. *13. KIP-853 also extends the tools to support a new feature kraft.version.* *It would be useful to have alignment between that KIP and this one.* Sounds good. Looks like Jose is in on the discussion so we can continue here. :) Jose -- *1. KIP-853 uses --feature for kafka-storage instead of --features.* *This is consistent with the use of --feature in the "kafka-feature.sh* *upgrade" command.* I wanted to include multiple features in one command, so it seems like features is a better name. I discuss more below about why I think we should allow setting multiple features at once. *2. I find it a bit inconsistent that --feature and --release-version* *are mutually exclusive in the kafka-feature CLI but not in the* *kafka-storage CLI. What is the reason for this decision?* For the storage tool, we are setting all the features for the cluster. By default, all are set. For the upgrade tool, the default is to set one feature. In the storage tool, it is natural for the --release-version to set the remaining features that --features didn't cover since otherwise we would need to set them all If we use the flag. In the feature upgrade case, it is less necessary for all the features to be set at once and the tool can be run multiple times. I'm not opposed to allowing both flags, but it is less necessary in my opinion. *3. KIP-853 deprecates --metadata in the kafka-features and makes it an* *alias for --release-version. In KIP-1022, what happens if the user* *specified both --metadata and --feature?* See my note above (Jun's comment 12) about deprecating old commands. I would think as the KIP stands now, we would not accept both commands. *4. I would suggest keeping this* *consistent with kafka-features. It would avoid having to implement one* *more parser in Kafka.* I sort of already implemented it as such, so I don't think it is too tricky. I'm not sure of an alternative. Kafka features currently only supports one feature at a time. I would like to support more than one for the storage tool. Do you have another suggestion for multiple features in the storage tool? *5. As currently described, trial and error seem to be the* *only mechanism. Should the Kafka documentation describe these* *dependencies? Is that good enough?* The plan so far is documentation. The idea is that this is an advanced feature, so I think it is reasonable to ask folks use documentation *6. Did you mean that 3.8-IV4 would map to TV2? If* *not, 3.8-IV3 would map to two different TV values.* It was a typo. Each MV maps to a single other feature version. *7. For --release-version in "kafka-features upgrade", does* *--release-version mean that all of the feature versions will either* *upgrade or stay the same? Meaning that downgrades will be rejected by* *the Kafka controller. How about when --release-version is used with* *"kafka-features downgrade"?* The idea I had was that the cluster will check if all the versions are supported. If any version is not supported the tool will throw an error. We can also issue a warning if the given command (if supported by the cluster) will downgrade a feature. One option is also to require a yes/no prompt or flag to allow downgrades. The downgrade tool would allow the same.
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi Justine and Jun, Thanks for the KIP Justine. See my comments below. On Wed, Feb 28, 2024 at 3:09 PM Jun Rao wrote: > 13. KIP-853 also extends the tools to support a new feature kraft.version. > It would be useful to have alignment between that KIP and this one. I agree. I took a look at this KIP and these are the differences that I can spot. 1. KIP-853 uses --feature for kafka-storage instead of --features. This is consistent with the use of --feature in the "kafka-feature.sh upgrade" command. 2. I find it a bit inconsistent that --feature and --release-version are mutually exclusive in the kafka-feature CLI but not in the kafka-storage CLI. What is the reason for this decision? 3. KIP-853 deprecates --metadata in the kafka-features and makes it an alias for --release-version. In KIP-1022, what happens if the user specified both --metadata and --feature? 4. There is an example: "kafka-storage format --features metadata.version=16,transaction.version=2,group.coordinator.version=1". This is different from the --feature flag in kafka-features which is an append flag. Meaning that multiple features are specified as "--feature metadata.version=16 --feature transaction.version=2 --feature group.coordinator.version=1". I would suggest keeping this consistent with kafka-features. It would avoid having to implement one more parser in Kafka. 5. In the section "A Note on Feature Interdependence", you mention "an error should be thrown if trying to format with X version 13 and Y version 12". How would the user discover (or describe) these dependencies? As currently described, trial and error seem to be the only mechanism. Should the Kafka documentation describe these dependencies? Is that good enough? 6. In "3.8-IV2 that could map to TV1, 3.8-IV3 could also be TV1, and 3.8-IV3 could be TV2" did you mean that 3.8-IV4 would map to TV2? If not, 3.8-IV3 would map to two different TV values. 7. For --release-version in "kafka-features upgrade", does --release-version mean that all of the feature versions will either upgrade or stay the same? Meaning that downgrades will be rejected by the Kafka controller. How about when --release-version is used with "kafka-features downgrade"? -José
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi, Justine, Thanks for the KIP. A few comments below. 10. Currently, MV controls records, inter-broker RPCs and code logic. With more features, would each of those be controlled by a separate feature or multiple features. For example, is the new transaction record format controlled only by MV with TV having a dependency on MV or is it controlled by both MV and TV. 11. "If not all features are covered with this flag, the command will just use the latest production version of the feature like it does for metadata version." Should we apply that logic to --release-version too? Basically, if --release-version is not used, the command will just use the latest production version of every feature. Should we apply that logic to both tools? 12. Should we remove --metadata METADATA from kafka-features? It does the same thing as --release-version. 13. KIP-853 also extends the tools to support a new feature kraft.version. It would be useful to have alignment between that KIP and this one. Jun On Wed, Feb 28, 2024 at 10:49 AM Artem Livshits wrote: > Hi Justine, > > Thank you for the KIP. I think the KIP is pretty clear and makes sense to > me. Maybe it would be good to give a little more detail on the > implementation of feature mapping and how the tool would validate the > feature combinations. For example, I'd expect that > > bin/kafka-storage.sh format --release-version 3.6-IVI --feature > transaction.version=2 > > would give an error because the new transaction protocol is not supported > in 3.6. Also, we may decide that > > bin/kafka-storage.sh format --release-version 5.0-IV0 --feature > transaction.version=0 > > would be an unsupported combination as it'll have been a while since the > new transaction protocol has been the default and it would be too risky to > enable this combination as it may not be tested any more. > > As for the new names, I'm thinking of the "transaction feature version" > more like a "transaction protocol version" -- from the user perspective we > don't really add new functionality in KIP-890, we're changing the protocol > to be more robust (and potentially faster). > > -Artem > > > > On Wed, Feb 28, 2024 at 10:08 AM Justine Olshan > wrote: > > > Hey Andrew, > > > > Thanks for taking a look. > > > > I previously didn't include 1. We do plan to use these features > immediately > > for KIP-890 and KIP-848. If we think it is easier to put the discussion > in > > those KIP discussions we can, but I fear that it will easily get lost > given > > the size of the KIPs. > > > > I named the features similar to how we named metadata version. > Transaction > > version would control transaction features like enabling a new > transaction > > record format and APIs to enable KIP-890 part 2. Likewise, the group > > coordinator version would also enable the new record formats there and > the > > new group coordinator. I am open to new names or further discussion. > > > > For 2 and 3, I can provide example scripts that show the usage. I am open > > to adding --latest-stable as well. > > > > Justine > > > > On Tue, Feb 27, 2024 at 4:59 AM Andrew Schofield < > > andrew_schofield_j...@outlook.com> wrote: > > > > > Hi Justine, > > > Thanks for the KIP. This area of Kafka is complicated and making it > > easier > > > is good. > > > > > > When I use the `kafka-features.sh` tool to describe the features on my > > > cluster, I find that there’s a > > > single feature called “metadata.version”. I think this KIP does a > handful > > > of things: > > > > > > 1) It introduces the idea of two new features, TV and GCV, without > giving > > > them concrete names or > > > describing their behaviour. > > > 2) It introduces a new flag on the storage tool to enable advanced > users > > > to control individual features > > > when they format storage for a new broker. > > > 3) It introduces a new flag on the features tool to enable a set of > > latest > > > stable features for a given > > > version to be enabled all together. > > > > > > I think that (1) probably shouldn’t be in this KIP unless there are > > > concrete details. Maybe this KIP is enabling > > > the operator experience when we introduce TV and GCV in other KIPs. I > > > don’t believe the plan is to enable > > > the new group coordinator with a feature, and it seems unnecessary to > me. > > > I think it’s more compelling for TV > > > given the changes in transactions. > > > > > > For (2) and (3), it would be helpful to explicit about the syntax for > the > > > enhancements to the tool. I think > > > that for the features tool, `--release-version` is an optional > parameter > > > which requires a RELEASE_VERSION > > > argument. I wonder whether it would be helpful to have > `--latest-stable` > > > as an option too. > > > > > > Thanks, > > > Andrew > > > > > > > On 26 Feb 2024, at 21:26, Justine Olshan > > > > > > wrote: > > > > > > > > Hello folks, > > > > > > > > I'm proposing a KIP that allows for setting and upgrading new > features > > > >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi Justine, Thank you for the KIP. I think the KIP is pretty clear and makes sense to me. Maybe it would be good to give a little more detail on the implementation of feature mapping and how the tool would validate the feature combinations. For example, I'd expect that bin/kafka-storage.sh format --release-version 3.6-IVI --feature transaction.version=2 would give an error because the new transaction protocol is not supported in 3.6. Also, we may decide that bin/kafka-storage.sh format --release-version 5.0-IV0 --feature transaction.version=0 would be an unsupported combination as it'll have been a while since the new transaction protocol has been the default and it would be too risky to enable this combination as it may not be tested any more. As for the new names, I'm thinking of the "transaction feature version" more like a "transaction protocol version" -- from the user perspective we don't really add new functionality in KIP-890, we're changing the protocol to be more robust (and potentially faster). -Artem On Wed, Feb 28, 2024 at 10:08 AM Justine Olshan wrote: > Hey Andrew, > > Thanks for taking a look. > > I previously didn't include 1. We do plan to use these features immediately > for KIP-890 and KIP-848. If we think it is easier to put the discussion in > those KIP discussions we can, but I fear that it will easily get lost given > the size of the KIPs. > > I named the features similar to how we named metadata version. Transaction > version would control transaction features like enabling a new transaction > record format and APIs to enable KIP-890 part 2. Likewise, the group > coordinator version would also enable the new record formats there and the > new group coordinator. I am open to new names or further discussion. > > For 2 and 3, I can provide example scripts that show the usage. I am open > to adding --latest-stable as well. > > Justine > > On Tue, Feb 27, 2024 at 4:59 AM Andrew Schofield < > andrew_schofield_j...@outlook.com> wrote: > > > Hi Justine, > > Thanks for the KIP. This area of Kafka is complicated and making it > easier > > is good. > > > > When I use the `kafka-features.sh` tool to describe the features on my > > cluster, I find that there’s a > > single feature called “metadata.version”. I think this KIP does a handful > > of things: > > > > 1) It introduces the idea of two new features, TV and GCV, without giving > > them concrete names or > > describing their behaviour. > > 2) It introduces a new flag on the storage tool to enable advanced users > > to control individual features > > when they format storage for a new broker. > > 3) It introduces a new flag on the features tool to enable a set of > latest > > stable features for a given > > version to be enabled all together. > > > > I think that (1) probably shouldn’t be in this KIP unless there are > > concrete details. Maybe this KIP is enabling > > the operator experience when we introduce TV and GCV in other KIPs. I > > don’t believe the plan is to enable > > the new group coordinator with a feature, and it seems unnecessary to me. > > I think it’s more compelling for TV > > given the changes in transactions. > > > > For (2) and (3), it would be helpful to explicit about the syntax for the > > enhancements to the tool. I think > > that for the features tool, `--release-version` is an optional parameter > > which requires a RELEASE_VERSION > > argument. I wonder whether it would be helpful to have `--latest-stable` > > as an option too. > > > > Thanks, > > Andrew > > > > > On 26 Feb 2024, at 21:26, Justine Olshan > > > wrote: > > > > > > Hello folks, > > > > > > I'm proposing a KIP that allows for setting and upgrading new features > > > (other than metadata version) via the kafka storage format and feature > > > tools. This KIP extends on the feature versioning changes introduced by > > > KIP-584 by allowing for the features to be set and upgraded. > > > > > > Please take a look: > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1023%3A+Formatting+and+Updating+Features > > > > > > Thanks, > > > > > > Justine > > > > >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hey Andrew, Thanks for taking a look. I previously didn't include 1. We do plan to use these features immediately for KIP-890 and KIP-848. If we think it is easier to put the discussion in those KIP discussions we can, but I fear that it will easily get lost given the size of the KIPs. I named the features similar to how we named metadata version. Transaction version would control transaction features like enabling a new transaction record format and APIs to enable KIP-890 part 2. Likewise, the group coordinator version would also enable the new record formats there and the new group coordinator. I am open to new names or further discussion. For 2 and 3, I can provide example scripts that show the usage. I am open to adding --latest-stable as well. Justine On Tue, Feb 27, 2024 at 4:59 AM Andrew Schofield < andrew_schofield_j...@outlook.com> wrote: > Hi Justine, > Thanks for the KIP. This area of Kafka is complicated and making it easier > is good. > > When I use the `kafka-features.sh` tool to describe the features on my > cluster, I find that there’s a > single feature called “metadata.version”. I think this KIP does a handful > of things: > > 1) It introduces the idea of two new features, TV and GCV, without giving > them concrete names or > describing their behaviour. > 2) It introduces a new flag on the storage tool to enable advanced users > to control individual features > when they format storage for a new broker. > 3) It introduces a new flag on the features tool to enable a set of latest > stable features for a given > version to be enabled all together. > > I think that (1) probably shouldn’t be in this KIP unless there are > concrete details. Maybe this KIP is enabling > the operator experience when we introduce TV and GCV in other KIPs. I > don’t believe the plan is to enable > the new group coordinator with a feature, and it seems unnecessary to me. > I think it’s more compelling for TV > given the changes in transactions. > > For (2) and (3), it would be helpful to explicit about the syntax for the > enhancements to the tool. I think > that for the features tool, `--release-version` is an optional parameter > which requires a RELEASE_VERSION > argument. I wonder whether it would be helpful to have `--latest-stable` > as an option too. > > Thanks, > Andrew > > > On 26 Feb 2024, at 21:26, Justine Olshan > wrote: > > > > Hello folks, > > > > I'm proposing a KIP that allows for setting and upgrading new features > > (other than metadata version) via the kafka storage format and feature > > tools. This KIP extends on the feature versioning changes introduced by > > KIP-584 by allowing for the features to be set and upgraded. > > > > Please take a look: > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1023%3A+Formatting+and+Updating+Features > > > > Thanks, > > > > Justine > >
Re: [DISCUSS] KIP-1022 Formatting and Updating Features
Hi Justine, Thanks for the KIP. This area of Kafka is complicated and making it easier is good. When I use the `kafka-features.sh` tool to describe the features on my cluster, I find that there’s a single feature called “metadata.version”. I think this KIP does a handful of things: 1) It introduces the idea of two new features, TV and GCV, without giving them concrete names or describing their behaviour. 2) It introduces a new flag on the storage tool to enable advanced users to control individual features when they format storage for a new broker. 3) It introduces a new flag on the features tool to enable a set of latest stable features for a given version to be enabled all together. I think that (1) probably shouldn’t be in this KIP unless there are concrete details. Maybe this KIP is enabling the operator experience when we introduce TV and GCV in other KIPs. I don’t believe the plan is to enable the new group coordinator with a feature, and it seems unnecessary to me. I think it’s more compelling for TV given the changes in transactions. For (2) and (3), it would be helpful to explicit about the syntax for the enhancements to the tool. I think that for the features tool, `--release-version` is an optional parameter which requires a RELEASE_VERSION argument. I wonder whether it would be helpful to have `--latest-stable` as an option too. Thanks, Andrew > On 26 Feb 2024, at 21:26, Justine Olshan wrote: > > Hello folks, > > I'm proposing a KIP that allows for setting and upgrading new features > (other than metadata version) via the kafka storage format and feature > tools. This KIP extends on the feature versioning changes introduced by > KIP-584 by allowing for the features to be set and upgraded. > > Please take a look: > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1023%3A+Formatting+and+Updating+Features > > Thanks, > > Justine
