Hi, Ismael, Sounds good. The KIP looks good to me.
Thanks, Jun On Wed, Jun 9, 2021 at 9:16 AM Ismael Juma <ism...@juma.me.uk> wrote: > Thanks Jun. After further thought and discussion with Jason, we decided to > move `log.record.version.force.upgrade` to a "Future Work" section. > > There are more details in the KIP, but there are some complexities in > making it possible to remove read support for v0/v1 formats. Given that, we > decided to focus on the items that have to happen by 3.0 in this KIP: > ensuring all new client writes use v2. The rest can happen via a subsequent > KIP if we can devise a good solution. > > I will start the vote thread in parallel, but happy to discuss further. > > Ismael > > On Tue, Jun 8, 2021 at 9:19 AM Jun Rao <j...@confluent.io.invalid> wrote: > > > Hi, Ismael, > > > > Thanks for the KIP. > > > > Regarding log.record.version.force.upgrade: > > 1. Is that a topic level or a broker level config? The slight benefit of > > topic level is that it allows people to upgrade incrementally. The > > potential downside is the additional complexity associated with it. > > 2. When log.record.version.force.upgrade is set to 2, do we need to scan > > all batches in all existing segments during broker restart to verify the > > record format? Since this is rarely needed, the performance impact may > not > > be a big concern. However, it will probably be useful to document this in > > the KIP. > > 3. Related to 2, we need a way to know when the upgrading to v2 message > > format has completed. This could be as simple as some logging. It would > be > > useful to document this in the KIP. > > 4. Related to 2, once the upgrade to v2 message format has completed, do > we > > expect the user to set log.record.version.force.upgrade to null? > Otherwise, > > we will pay the batch scanning overhead on every broker restart. It would > > be useful to document the upgrade process related to this. > > > > Thanks, > > > > Jun > > > > > > On Tue, Jun 8, 2021 at 8:21 AM Ismael Juma <ism...@juma.me.uk> wrote: > > > > > Hi Jason, > > > > > > Thanks for the thoughtful comments. Responses below. > > > > > > 1. Good point. I updated the KIP to up-convert during replication if > the > > > IBP is 3.0. I am still not 100% sure if this is truly required, but it > > does > > > seem like a good idea to avoid flip-flopping like this. I'll follow up > > with > > > you to understand if this is a correctness requirement or something > that > > > makes the system easier to reason about and then update the KIP and > this > > > thread. > > > > > > 2. Agreed. I updated the KIP to mention that we will up-convert to > single > > > record batches for the uncompressed case. > > > > > > 3. I added a note regarding produce request versions we will no longer > > > support. I originally did not include this since the broker doesn't > seem > > to > > > do the validation for produce requests while it does for fetch > requests. > > > But from a specification perspective, it makes sense to include it > since > > > the current behavior is confusing (it may be this way for compatibility > > > with now very old clients). With regards to list offsets v0, I suggest > we > > > do that via a separate KIP. As you said, there might be other similar > > > opportunities and since the target would be AK 4.0, we have a bit of > time > > > to do the analysis. > > > > > > Ismael > > > > > > On Fri, Jun 4, 2021 at 2:10 PM Jason Gustafson > > <ja...@confluent.io.invalid > > > > > > > wrote: > > > > > > > Hi Ismael, > > > > > > > > I agree it would be awesome to drop support for the old formats! A > few > > > > comments below: > > > > > > > > 1. Suppose that a partition with 3 replicas begins at v0. One broker > is > > > > upgraded to v2 with the new "force upgrade" config as part of a > rolling > > > > restart, which leaves two replicas on v0 and one on v2. The KIP > > documents > > > > that replicas will take the format from the leader without any > > > conversion, > > > > so I'm a little concerned that leader changes in this state could > > result > > > in > > > > the version flipping from v2 to v0. Could that happen? Say, for > > example, > > > if > > > > the third replica was catching up while the rolling restart was in > > > > progress. Since it is probably an unlikely case in practice, would it > > > make > > > > sense to allow up-conversion during replication when "force upgrade" > > has > > > > been enabled? Not sure I see any great alternatives to this. > > > > > > > > 2. A related question concerns batching when upgrading from the > > > non-batched > > > > formats (i.e. v0 and v1 when no compression is in use). I suspect the > > > only > > > > reasonable way to do this is to treat each individual record as a > batch > > > of > > > > 1 during the conversion process. Otherwise, I am not sure we can come > > up > > > > with a deterministic approach to re-batching which can be applied > > > > consistently on all replicas (especially taking into account > > compaction). > > > > The reason this is important is that replication is aligned by > batches, > > > so > > > > if the batches are not consistent, the replication won't work. The > only > > > > problem with using single-record batches is that the v2 message > format > > > has > > > > some additional overhead for the batch itself when compared with the > v0 > > > and > > > > v1 individual record format. So the up-conversion process would > likely > > > > increase storage. I don't think this is a dealbreaker, but probably > > worth > > > > calling out somewhere. > > > > > > > > 3. In 4.0, we are dropping support for Fetch versions 0 through 3. I > > > might > > > > have missed it in the KIP, but it sounds like we need a similar > > > restriction > > > > on Produce versions since up-conversion will also not be supported? > > Would > > > > it be reasonable to drop ListOffsets v0 at the same time? It is not > > > related > > > > to the message format, but the Fetch restriction will already > > effectively > > > > kill any client that might still be using this wonky version. Not > sure > > if > > > > there are any similar opportunities, but might be worth checking. > > > > > > > > Thanks, > > > > Jason > > > > > > > > > > > > > > > > On Fri, Jun 4, 2021 at 6:14 AM Ismael Juma <ism...@juma.me.uk> > wrote: > > > > > > > > > If there are no comments or objections, I will start a vote on this > > > soon. > > > > > > > > > > On Tue, Jun 1, 2021 at 7:59 AM Ismael Juma <ism...@juma.me.uk> > > wrote: > > > > > > > > > > > Hi all, > > > > > > > > > > > > It's time to start the process of sunsetting message formats v0 > and > > > v1 > > > > in > > > > > > order to establish a new baseline in terms of supported > > client/broker > > > > > > behavior and to improve maintainability & supportability of > Kafka. > > > > Please > > > > > > take a look at the proposal: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-724%3A+Drop+support+for+message+formats+v0+and+v1 > > > > > > > > > > > > Ismael > > > > > > > > > > > > > > > > > > > > >
