Hi Yi, > Does it mean with a flexible version, the future version of these value types will stay at the version where the flexibility is first introduced? Will there be any need to bump the version again in the future?
Yes, there will be no need to bump the version since we will only be adding tagged fields but in the chance the version is bumped, we will deserialize to the highest known (flexible) version which will ignore unknown tagged fields. > To enforce the version not bumping, is it possible to have a unit test to gate? Do you have some tests in mind? I don't think we can tell whether a version was bumped or not. Best, Jeff On Thu, Mar 23, 2023 at 12:07 PM Yi Ding <yd...@confluent.io.invalid> wrote: > Hi Jeff, > > Thanks for the update. Does it mean with a flexible version, the future > version of these value types will stay at the version where the flexibility > is first introduced? Will there be any need to bump the version again in > the future? > To enforce the version not bumping, is it possible to have a unit test to > gate? > > > On Wed, Mar 22, 2023 at 3:19 PM Jeff Kim <jeff....@confluent.io.invalid> > wrote: > > > Hi all, > > > > After discussing with my colleagues, I have repurposed the KIP for a > > general downgrade solution for both transaction and group coordinators. > The > > KIP no longer discusses the downgrade path but instead lays out the > > foundation for future downgrade solutions. > > > > Link: > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-915%3A+Txn+and+Group+Coordinator+Downgrade+Foundation > > > > Thanks, > > Jeff > > > > On Mon, Mar 20, 2023 at 7:37 PM Jeff Kim <jeff....@confluent.io> wrote: > > > > > Hi David and Justine, > > > > > > Thank you both for the detailed feedback. > > > > > > David, > > > > > > 1. That makes sense. I revised the "Reading new fields" section with > how > > > we can downgrade to the highest known version and that this was > confirmed > > > via unit testing. I also attempted to dive deeper into using tagged > > fields > > > and the rejected alternative. Please let me know what you think. > > > > > > 2. Under "Restrictions and Guidelines" I updated the paragraph to > clearly > > > state that we want to use tagged fields across all record types > > introduced > > > in KIP-848 including OffsetCommitValue. > > > > > > 3. Would it be possible to bump the OffsetCommitValue record version to > > > make it flexible along with the changes to parse with the highest known > > > version? I'm not sure I understand why we cannot make both changes > > together. > > > > > > 4. I completely missed this. Added some notes at the end of > "Restrictions > > > and Guidelines". Unfortunately I can't think of a solution at the > moment. > > > Will get back to you. > > > > > > 5. I have a section under "Identifying New Record Types" that discusses > > > this: > > > > We can automate the cleanup by writing tombstones when the > coordinator > > > reads unrecognizable records. This may add duplicate work if tombstones > > > were already added but not yet pruned by the log cleaner. > > > This is a sure way to delete any unknown record types even if the > > operator > > > does not follow the steps. > > > > > > 6. Thanks, I have expanded on the section on transactional offset > > commits. > > > As for log compaction, my understanding was that we can control the > > process > > > by forcing compaction. Is my understanding incorrect? > > > > > > 7. We throw an exception if ConfigResource.type is unknown which is > true > > > in our case because KIP-848 introduces a new GROUP resource type. We > will > > > need to add some sort of safeguard if the dynamic configs are not > deleted > > > before the server downgrade. I'll give you an update once I update the > > > section. > > > > > > Justine, > > > > > > On the transactional offset commits, I have updated the KIP to reflect > > > your points after our offline discussion (much appreciated). It seems > > that > > > the work required to abort from the server side is fairly big and will > > > require additional investigation if we are to go down this path. > > > > > > As for downgrade limitations, I missed that part so thanks for the > catch > > > (along with David). Unfortunately, the proposed design won't allow > > > downgrades even after the new record types are deleted because the > > existing > > > OffsetCommitValue record is not flexible and will not be able to parse > > > the topicId tagged field. I'll think more about this and get back to > you. > > > > > > Best, > > > Jeff > > > > > > On Mon, Mar 20, 2023 at 2:16 PM Justine Olshan > > > <jols...@confluent.io.invalid> wrote: > > > > > >> Hey Jeff and David, > > >> > > >> Thanks for the KIP! I was also looking into this a bit since I may > want > > to > > >> change the record format for KIP-890 as well (finally implementing the > > >> record change from KIP-360 to better support the epoch bump. This will > > >> potentially be helpful for me to implement that work. > > >> > > >> I discussed some aspects with David offline. Namely, the alternative > > >> solution to rewrite the records in the old format upon downgrade. One > > plus > > >> to this approach is that it should trigger compaction for all the > > existing > > >> (new format) records. > > >> The main issue we ran into was how to handle ongoing transactions. > (Ie, > > >> could we abort and rewrite with the old format) I think that's one of > > the > > >> main downsides. I think it could potentially be possible to do this, > but > > >> we'd definitely need a server-side mechanism to abort and rewrite the > > >> records. > > >> > > >> I think the trouble I was running into with the current solution is > that > > >> we > > >> can only downgrade to a version slightly before the new group > > coordinator. > > >> If someone was running 3.2 and they upgrade to 3.6, but find a bug in > > 3.4 > > >> (or any version between 3.5 and the one they upgraded from), then they > > can > > >> only downgrade to 3.5. This puts us in a difficult spot. I guess in > this > > >> scenario, they will need to wait for the new format records to get > > cleared > > >> away before downgrading again. Is that correct? > > >> > > >> Thanks, > > >> Justine > > >> > > >> On Thu, Mar 16, 2023 at 10:41 AM David Jacot > > <dja...@confluent.io.invalid > > >> > > > >> wrote: > > >> > > >> > Hi Jeff, > > >> > > > >> > Thanks for the KIP! I am really glad that we are finally addressing > > this > > >> > gap in KIP-848. I have a few general comments. > > >> > > > >> > 1. Overall, I feel like the important bits are not bold enough in > the > > >> KIP. > > >> > I think that it is good to explain the overall upgrade/downgrade > > process > > >> > and to highlight where the issues are but I think that the core bits > > >> should > > >> > give more details. For instance, we should explain why relying on > tag > > >> > fields works to ignore fields added in future releases. My > > >> understanding is > > >> > that it works because the buffer for the tagged fields is serialized > > at > > >> the > > >> > end so reading with the old version, which is a prefix of the new > one, > > >> > works. > > >> > > > >> > 2. Moreover, it would be great if we could make the principle more > > >> general. > > >> > My hope is that we can keep reusing the principles introduced in the > > >> KIP in > > >> > future releases as well. For instance, let's say that we need to > add a > > >> new > > >> > field to one of the new records introduced by KIP-848 or that we > will > > >> have > > >> > to introduce a new record type as well. Would it work for those > cases > > as > > >> > well? > > >> > > > >> > 3. Regarding enabling support for tagged fields for the > > >> OffsetCommitValue > > >> > record, it would be great if you could give more details on the > steps > > to > > >> > get there in the KIP. My understanding is that we would have to do > the > > >> > following: 1) Update the code which reads the records to fail back > to > > >> the > > >> > highest known version if the version stored in the log is unknown. > > Let's > > >> > say that we do this in AK 3.5. 2) We need to turn on tagged fields > for > > >> the > > >> > record. I think that we can only do this in AK 3.6+. > > >> > > > >> > 4. I may have missed this part but we should clearly explain the > > >> drawback > > >> > of the proposed approach as well. Say that we enable tagged fields > for > > >> > OffsetCommitValue in AK 3.6. This means that it won't be possible to > > >> > downgrade a cluster from 3.6 to a version earlier than 3.5. This is > a > > >> > significant limitation in my opinion because, I think, users don't > > >> > necessarily upgrade to all versions. > > >> > > > >> > 5. In the proposal, it is not clear about whether the old software > > will > > >> > delete unknown records or not. It is true that new records will be > > >> deleted > > >> > when the group is downgraded but this only works if the operator > > >> respects > > >> > the process. > > >> > > > >> > 6. It would be great if we could extend the rejected alternative. > The > > >> > alternative sounds clearly better when you read it so we should > really > > >> > explain the reason to reject it. 1) One issue that you mention is > that > > >> the > > >> > log must be compacted before downgrading and we don't really control > > >> this > > >> > process. 2) Transactions may be difficult to handle. I suppose that > it > > >> is > > >> > possible to handle them though. Have you thought about this? > > >> > > > >> > 7. For the new dynamic configs, what happens if they are kept and > the > > >> > quorum controller is downgraded? > > >> > > > >> > Best, > > >> > David > > >> > > > >> > On Thu, Mar 16, 2023 at 12:56 AM Jeff Kim > > <jeff....@confluent.io.invalid > > >> > > > >> > wrote: > > >> > > > >> > > Hi folks, > > >> > > > > >> > > I would like to start a discussion thread for KIP-915: Next Gen > > Group > > >> > > Coordinator Downgrade Path which proposes the downgrade design for > > the > > >> > new > > >> > > group coordinator introduced in KIP-848 > > >> > > < > > >> > > > > >> > > > >> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-848%3A+The+Next+Generation+of+the+Consumer+Rebalance+Protocol > > >> > > > > > >> > > . > > >> > > > > >> > > KIP: > > >> > > > > >> > > > > >> > > > >> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-915%3A+Next+Gen+Group+Coordinator+Downgrade+Path > > >> > > > > >> > > Thanks, > > >> > > Jeff > > >> > > > > >> > > > >> > > > > > >
