Thanks for pointing this out Chris. I have updated the KIP with the correct sequence of steps.
Thanks, Ashwin On Wed, Dec 6, 2023 at 11:48 PM Chris Egerton <chr...@aiven.io.invalid> wrote: > Hi Ashwin, > > Regarding point 4--I think this is still a bit unwise. When workers pick up > a new connector config from the config topic, they participate in a > rebalance. It may be safe to write offsets during that rebalance, but in > the name of simplicity, do you think we can write the offsets for the > connector before its config? The sequence of steps would be something like > this: > > 1. Validate offsets and connector config (can be done in any order) > 2. Write offsets > 3. Write connector config (with whatever initial state is specified in the > request, or the default if none is specified) > > Cheers, > > Chris > > On Wed, Dec 6, 2023 at 9:13 AM Ashwin <apan...@confluent.io.invalid> > wrote: > > > Hello Chris, > > > > Thanks for the quick and detailed review. Please see my responses below > > > > High-level thoughts: > > > > 1. I had not thought of this till now, thanks for pointing it out. I > > would lean towards the second option of cleaning previous offsets as > > it will result in the fewer surprises for the user. > > > > 2 and 3. I agree and have updated the wiki to that effect. I just > > wanted to use the connector name as a mutex - we can handle the race > > in other ways. > > > > 4. Yes, I meant submitting config to the config topic. Have updated > > the wiki to make it clearer. > > > > > > Nits: > > > > Thanks for pointing these - I have made the necessary changes. > > > > > > Thanks again, > > > > Ashwin > > > > > > On Mon, Dec 4, 2023 at 9:05 PM Chris Egerton <chr...@aiven.io.invalid> > > wrote: > > > > > Hi Ashwin, > > > > > > Thanks for the KIP! This would be a nice simplification to the process > > for > > > migrating connectors enabled by KIP-980, and would also add global > > support > > > for a feature I've seen implemented by hand in at least a couple > > connectors > > > over the years. > > > > > > > > > High-level thoughts: > > > > > > 1. If there are leftover offsets from a previous connector, what will > the > > > impact on those offsets (if any) be if a new connector is created with > > the > > > same name with initial offsets specified? I can think of at least two > > > options: we leave those offsets as-are but allow any of the initial > > offsets > > > in the new connector request to overwrite them, or we automatically > wipe > > > all existing offsets for the connector first before writing its initial > > > offsets and then creating it. I have a slight preference for the first > > > because it's simpler to implement and aligns with existing precedent > for > > > offset handling where we never wipe them unless explicitly requested by > > the > > > user or connector, but it could be argued that the second is less > likely > > to > > > generate footguns for users. Interested in your thoughts! > > > > > > 2. IMO preflight validation (i.e., the "Validate initial_offset before > > > creating the connector in STOPPED state rejected alternative) is a > > > must-have for this kind of feature. I acknowledge the possible race > > > condition (and I don't think it's worth it to track in-flight connector > > > creation requests in the herder in order to prevent this race, since > > > ever-blocking Connector operations would be cumbersome to deal with), > and > > > the extra implementation effort. But I don't think either of those tip > > the > > > scales far enough to override the benefit of ensuring the submitted > > offsets > > > are valid before creating the connector. > > > > > > 3. On the topic of preflight validation--I also think it's important > that > > > we validate both the connector config and the initial offsets before > > either > > > creating the connector or storing the initial offsets. I don't think > this > > > point requires any changes to the KIP that aren't already proposed with > > > point 2. above, but wanted to see if we could adopt this as a primary > > goal > > > for the design of the feature and keep it in mind with future changes. > > > > > > 4. In the proposed changes section, the first step is "Create a > connector > > > in STOPPED state", and the second step is "Validate the offset...". > What > > > exactly is entailed by "Create a connector"? Submit the config to the > > > config topic (presumably after a preflight validation of the config)? > > > Participate in the ensuing rebalance? I'm a little hesitant to start > > > performing rebalances inside REST requests, wondering if we can find a > > > lighter-weight way to implement this. > > > > > > > > > Nits: > > > > > > 5. You can remove the italicized "This page is meant as a template for > > > writing a KIP" section after the table of contents. > > > > > > 6. Can you file a JIRA ticket for this change and add a link to it in > the > > > KIP under the status section? > > > > > > 7. What do you think about changing the name for the new field from > > > "initial_offset" (singular) to "initial_offsets" (plural)? This is > > > especially relevant for connectors that read from multiple source > > > partitions, like MM2 and the various JDBC source connectors out there. > > > > > > 8. IMO it's not necessary to include this section: "Please note that > sink > > > and source connectors have different schemas for offset." While it's > > > technically true that the fields of the objects inside the "partition" > > and > > > "offset" fields will likely differ between sink and source connectors, > > > they'll also likely differ in the exact same way across different sink > > > connectors. I think it's enough to link to the relevant section in > > KIP-875 > > > on the details for the format. > > > > > > 9. Instead of "Connector-defined source partition" and > "Connector-defined > > > source offset" in the comments for the sample connector creation body > > > (which aren't strictly accurate for sink connectors), can we say > > something > > > like "Source partition" and "Desired initial offset"? > > > > > > 10. In the compatibility section the KIP states that "This new feature > > will > > > use the current OffsetStorageWriter." IMO we should refrain from > > referring > > > to internal API class names in KIPs when possible, since those class > > names > > > may change and users may also mistakenly assume that they're part of > the > > > public API. Can we say something like "This new feature will reuse > > existing > > > logic for storing offsets" instead? > > > > > > > > > Thanks again for the KIP! > > > > > > Cheers, > > > > > > Chris > > > > > > On Thu, Nov 30, 2023 at 2:26 AM Ashwin <apan...@confluent.io.invalid> > > > wrote: > > > > > > > Hi all, > > > > > > > > I'd like to begin discussion on KIP-995 which proposes to allow users > > > > to specify initial offset as part of the request to create a > connector > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-995%3A+Allow+users+to+specify+initial+offsets+while+creating+connectors > > > > > > > > During the discussion for KIP-980 > > > > < > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-980%3A+Allow+creating+connectors+in+a+stopped+state > > > > >, > > > > which proposed the creation of connectors in STOPPED state, there was > > > > a suggestion to also allow setting the initial offset for a connector > > > > in the connector creation API. The proposal was deemed valid > > > > <https://lists.apache.org/thread/tq79l3975hy69wsycvwoh9n8gbp5j26f> > > > > (point no.4) but was deferred to a future KIP. This KIP proposes to > > > > implement that change and builds upon the changes introduced in > > > > KIP-875 < > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-875%3A+First-class+offsets+support+in+Kafka+Connect#KIP875:FirstclassoffsetssupportinKafkaConnect-Alteringoffsets(request) > > > > > > > > > and KIP-980 < > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-980%3A+Allow+creating+connectors+in+a+stopped+state > > > > >. > > > > > > > > Thanks, > > > > Ashwin > > > > > > > > > >
