Hi Yash,
Thanks for the in-depth discussion! Continuations here:
1. Regarding delimiters (dots vs. underscores), we use dots in connector
configs for all runtime-recognized properties, including but not limited to
connector.class, tasks.max, key.converter, key.converter.*, and
transforms.*.type. Regarding the choice of name--I think it's because the
concept here is different from the "state" that we display in the status
API that a different name is warranted. Users can't directly write the
connector's actual state, they can still only specify a target state. And
there's no guarantee that a connector will ever enter a target state
(whether it's specified at creation time or later), since a failure can
always occur.
2. It still seems simpler to emit one record instead of two, especially
since we're not guaranteed that the leader will be using a transactional
producer. I guess I'm just wary of knowingly putting the config topic in an
inconsistent state (target state present without accompanying connector
config), even if it's meant to be only for a brief period. Thinking about
it some more though, a one-record approach comes with drawbacks in the
downgrade scenario: older workers wouldn't know how to handle the new
config format and would just fall back to creating the connector in the
running state. I suppose we should favor the two-record approach since the
downgrade scenario is more likely than the other failure mode, but it'd be
nice if we could think of a way to satisfy both concerns. Not a blocker,
though.
3. Standalone mode has always supported the REST API, and so far FWICT
we've maintained feature parity between the two modes for everything except
exactly-once source connectors, which would have required significant
additional work since we'd have to add support for storing source connector
offsets in a Kafka topic instead of on local storage like we currently do.
I'd really prefer if we could try to maintain feature parity wherever
possible--one way we could possibly do that with this KIP is to also add
support for JSON files with standalone mode.
4. Yeah, no need to block on that idea since there are other use cases for
creating stopped connectors. We can treat it like the option to delete
offsets along with the connector discussed in KIP-875: punt for now,
possibly implement later pending user feedback and indication of demand.
Might be worth adding to a "Future work" section as an indication that we
haven't ruled it out (in which case it'd make sense as a rejected
alternative) but have chosen not to implement yet.
And I had one new thought that's pretty implementation-oriented but may
influence the design slightly:
6. Right now we write an empty set of task configs to the config topic when
handling requests to stop a connector in distributed mode. Do we need to do
the same when creating connectors in the stopped state, or add any other
special logic besides noting the new state in the config topic? Or is it
sufficient to write a non-running target state to the config topic and then
rely on existing logic to simply refuse to generate task configs for the
newly-created connector? Is there any chance that the lack of task configs
(as opposed to an empty list of task configs) in the config topic for a
connector that exists will cause issues?
Cheers,
Chris
On Tue, Oct 3, 2023 at 3:29 AM Yash Mayya <yash.ma...@gmail.com> wrote:
> Hi Chris,
>
> Thanks for taking a look at this KIP!
>
> 1. I chose to go with simply "state" as that exact term is already exposed
> via some of the existing REST API responses and would be one that users are
> already familiar with (although admittedly something like "initial_state"
> wouldn't be much of a jump). Since it's a field in the request body for the
> connector creation endpoint, wouldn't it be implied that it is the
> "initial" state just like the "config" field represents the "initial"
> configuration? Also, I don't think x.y has been established as the field
> naming convention in the Connect REST API right? From what I can tell, x_y
> is the convention being followed for fields in requests ("kafka_topic" /
> "kafka_partition" / "kafka_offset" in the offsets APIs for instance) and
> responses ("error_count", "kafka_cluster_id", "recommended_values" etc.).
>
> 2. The connector configuration record is currently used for both connector
> create requests as well as connector config update requests. Since we're
> only allowing configuring the target state for newly created connectors, I
> feel like it'll be a cleaner separation of concerns to use the existing
> records for connector configurations and connector target states rather
> than bundling the "state" and "state.v2" (or equivalent) fields into the
> connector configuration record. The additional write should be very minimal
> overhead and the two writes would be an atomic operation for Connect
> clusters that are using a transactional producer for the config topic
> anyway. Thoughts?
>
> 3. I was thinking that we'd support standalone mode via the same connector
> creation REST API endpoint changes (addition of the "state" field). If
> there is further interest in adding similar support to the command line
> method of creating connectors as well, perhaps we could do so in a small
> follow-on KIP? I feel like ever since the standalone mode started
> supporting the full Connect REST API, the command line method of creating
> connectors is more of a "legacy" concept.
>
> 4. Yeah, connector / offsets migration was just used as a representative
> example of how this feature could be useful - I didn't intend for it to be
> the sole purpose of this KIP. However, that said, I really like the idea of
> accepting an "offsets" field in the connector creation request since it'd
> reduce the number of user operations required from 3 (create the connector
> in a STOPPED state; alter the offsets; resume the connector) to 1. I'd be
> happy to either create or review a separate small KIP for that if it sounds
> acceptable to you?
>
> 5. Whoops, thanks, I hadn't even noticed that that's where I had linked to.
> Fixed!
>
> Thanks,
> Yash
>
> On Mon, Oct 2, 2023 at 11:14 PM Chris Egerton <chr...@aiven.io.invalid>
> wrote:
>
> > Hi Yash,
> >
> > Thanks for the KIP! Frankly this feels like an oversight in 875 and I'm
> > glad we're closing that loop ASAP.
> >
> >
> > Here are my thoughts:
> >
> > 1. (Nit): IMO "start.state", "initial.state", or "target.state" might be
> > better than just "state" for the field name in the connector creation
> > request.
> >
> > 2. Why implement this in distributed mode with two writes to the config
> > topic? We could augment the existing format for connector configs in the
> > config topic [1] with a new field instead.
> >
> > 3. Although standalone mode is mentioned in the KIP, there's no detail on
> > the Java properties file format that we support for standalone mode
> (i.e.,
> > `connect-standalone config/connect-standalone.properties
> > config/connect-file-source.properties
> > config/connect-file-sink.properties`). Do we plan on adding support for
> > that mode as well?
> >
> > 4. I suspect there will be advantages for this feature beyond offsets
> > migration, but if offsets migration were the only motivation for it,
> > wouldn't it be simpler to accept an "offsets" field in connector creation
> > requests? That way, users wouldn't have to start a connector in a
> different
> > state and then resume it, they could just create the connector like
> normal.
> > I think the current proposal is acceptable, but wanted to float this
> > alternative in case we anticipate lots of connector migrations and want
> to
> > optimize for them a bit more.
> >
> > 5. (NIt): We can link to our own Javadocs [2] instead of javadoc.io
> >
> >
> > [1] -
> >
> >
> https://github.com/apache/kafka/blob/dcd8c7d05f2f22f2d815405e7ab3ad7439669239/connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaConfigBackingStore.java#L234-L236
> >
> > [2] -
> https://kafka.apache.org/35/javadoc/index.html?overview-summary.html
> >
> >
> > Cheers,
> >
> > Chris
> >
> > On Thu, Sep 21, 2023 at 2:37 AM Yash Mayya <yash.ma...@gmail.com> wrote:
> >
> > > Hi Ashwin,
> > >
> > > Thanks for taking a look and sharing your thoughts!
> > >
> > > 1. Yes, the request / response formats of the two APIs were
> intentionally
> > > made identical for such use-cases. [1]
> > >
> > > 2. I'm assuming you're referring to retaining the offset / config topic
> > > records for a connector when it is deleted by a user? Firstly, a
> > > connector's offsets aren't actually currently deleted when the
> connector
> > is
> > > deleted - it was listed as potential future work in KIP-875 [2].
> > Secondly,
> > > the config topic is the source of truth for the state of a Connect
> > cluster
> > > and a connector deletion is done by writing a null-valued record
> > > (tombstone) with the connector key to the config topic. We could
> > > potentially modify the delete mechanism to use a special new record
> > > (instead of a tombstone with the connector key) in order to retain the
> > > latest configuration of a connector while still deleting the actual
> > > connector - however, this would have its downsides and I don't see too
> > many
> > > benefits. Furthermore, connector migration between different Kafka
> > clusters
> > > was just used as a representational use case for creating connectors
> in a
> > > stopped state - it isn't the primary focus of this KIP as such.
> > >
> > > 3. KIP-875 goes into some more details about this [3], but the TLDR is
> > that
> > > the STOPPED state will be treated like the PAUSED state on older
> workers
> > > that don't recognize the STOPPED state.
> > >
> > > Thanks,
> > > Yash
> > >
> > > [1] -
> > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-875%3A+First-class+offsets+support+in+Kafka+Connect#KIP875:FirstclassoffsetssupportinKafkaConnect-Request/responseformat
> > >
> > > [2] -
> > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-875%3A+First-class+offsets+support+in+Kafka+Connect#KIP875:FirstclassoffsetssupportinKafkaConnect-Automaticallydeleteoffsetswithconnectors
> > >
> > > [3] -
> > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-875%3A+First-class+offsets+support+in+Kafka+Connect#KIP875:FirstclassoffsetssupportinKafkaConnect-STOPPEDtargetstate
> > >
> > > On Wed, Sep 20, 2023 at 7:24 PM Ashwin <apan...@confluent.io.invalid>
> > > wrote:
> > >
> > > > Thanks Yash! This is very useful for migrating connectors from one
> > > cluster
> > > > to another.
> > > >
> > > > I had the following comments/questions
> > > >
> > > > 1. Is the offset read using `GET /offsets` api always guaranteed to
> be
> > > in a
> > > > format accepted by `PATCH /offsets` ?
> > > > 2. I had to tackle a similar migration situation but the two connect
> > > > clusters in question were using the same backing Kafka cluster. The
> > > > challenge in this case is that when I delete the original connector,
> I
> > > want
> > > > to retain offsets and config topics. Do you think we should support
> > > > deletion of a connector without removal of these topics as part of
> this
> > > KIP
> > > > ?
> > > > 3. In the case of a downgrade, how will Connect worker handle the
> > > optional
> > > > “state” field in config topic ?
> > > >
> > > > Thanks,
> > > > Ashwin
> > > >
> > > >
> > > >
> > > >
> > > > On Sun, Sep 17, 2023 at 11:09 PM Yash Mayya <yash.ma...@gmail.com>
> > > wrote:
> > > >
> > > > > Hi all,
> > > > >
> > > > > I'd like to begin discussion on a KIP to allow creating connectors
> > in a
> > > > > stopped state -
> > > > >
> > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-980%3A+Allow+creating+connectors+in+a+stopped+state
> > > > >
> > > > >
> > > > > Thanks,
> > > > > Yash
> > > > >
> > > >
> > >
> >
>
