Hello Chris, Thanks for your feedback. I created the jira and also updated the description a bit mentioning that other similar race scenarios exist and the KIP is not trying to solve them.
Best, Ivan On Wed, Mar 27, 2024, at 17:08, Chris Egerton wrote: > Hi Ivan, > > Thanks for the updates. LGTM! > > RE atomicity: I think it should be possible and not _too_ invasive to > detect and handle these kinds of races by tracking the offset in the config > topic for connector configs and aborting an operation if that offset > changes between when the request was initiated and when the write to the > config topic will take place, but since the same kind of issue is also > possible with other connector operations (concurrent configuration PUTs, > for example) due to how validation is split out into a separate thread, I > agree that it's not worth blocking the KIP on fixing this. > > One final nit: Can you update the Jira ticket link in the KIP? > > Cheers, > > Chris > > On Wed, Mar 27, 2024 at 2:56 PM Ivan Yurchenko <i...@ivanyu.me> wrote: > > > Hi, > > > > I updated the KIP with the two following changes: > > 1. Using `null` values as tombstone value for removing existing fields > > from configuration. > > 2. Added a note about the lack of 100% atomicity, which seems very > > difficult to achieve practically. > > > > Ivan > > > > > > On Tue, Mar 26, 2024, at 14:45, Ivan Yurchenko wrote: > > > Speaking of the Chris' comment > > > > > > > One thought that comes to mind is that sometimes it may be useful to > > > > explicitly remove properties from a connector configuration. We might > > > > permit this by allowing users to specify null (the JSON literal, not a > > > > string containing the characters "null") as the value for to-be-removed > > > > properties. > > > > > > This actually makes sense. AFAIU, `null` cannot be in the connector > > config since https://github.com/apache/kafka/pull/11333, so using it as a > > tombstone value is a good idea. I can update the KIP. > > > > > > Ivan > > > > > > > > > On Tue, Mar 26, 2024, at 14:19, Ivan Yurchenko wrote: > > > > Hi all, > > > > > > > > This KIP is a bit old now :) but I think its context hasn't changed > > much since then and the KIP is still valid. I would like to finally bring > > it to some conclusion. > > > > > > > > Best, > > > > Ivan > > > > > > > > On 2021/07/12 14:49:47 Chris Egerton wrote: > > > > > Hi all, > > > > > > > > > > Know it's been a while for this KIP but in my personal experience > > the value > > > > > of a PATCH method in the REST API has actually increased over time > > and I'd > > > > > love to have this option for quick, stop-the-bleeding remediation > > efforts. > > > > > > > > > > One thought that comes to mind is that sometimes it may be useful to > > > > > explicitly remove properties from a connector configuration. We might > > > > > permit this by allowing users to specify null (the JSON literal, not > > a > > > > > string containing the characters "null") as the value for > > to-be-removed > > > > > properties. > > > > > > > > > > I'd love to see this change if you're still interested in driving > > it, Ivan. > > > > > Hopefully we can give it the attention it deserves in the upcoming > > months! > > > > > > > > > > Cheers, > > > > > > > > > > Chris > > > > > > > > > > On Fri, Jun 28, 2019 at 4:56 AM Ivan Yurchenko <iv...@gmail.com> > > > > > wrote: > > > > > > > > > > > Thank you for your feedback Ryanne! > > > > > > These are all surely valid concerns and PATCH isn't really > > necessary or > > > > > > suitable for normal production configuration management. However, > > there are > > > > > > cases where quick patching of the configuration is useful, such as > > hot > > > > > > fixes of production or in development. > > > > > > > > > > > > Overall, the change itself is really tiny and if the cost-benefit > > balance > > > > > > is positive, I'd still like to drive it further. > > > > > > > > > > > > Ivan > > > > > > > > > > > > On Wed, 26 Jun 2019 at 17:45, Ryanne Dolan <ry...@gmail.com> > > wrote: > > > > > > > > > > > > > Ivan, I looked at adding PATCH a while ago as well. I decided > > not to > > > > > > pursue > > > > > > > the idea for a few reasons: > > > > > > > > > > > > > > 1) PATCH is still racy. For example, if you want to add a topic > > to the > > > > > > > "topics" property, you still need to read, modify, and write the > > existing > > > > > > > value. To handle this, you'd need to support atomic sub-document > > > > > > > operations, which I don't see happening. > > > > > > > > > > > > > > 2) A common pattern is to store your configurations in git or > > something, > > > > > > > and then apply them via PUT. Throw in some triggers or jenkins > > etc, and > > > > > > you > > > > > > > have a more robust solution than PATCH provides. > > > > > > > > > > > > > > 3) For properties that change a lot, it's possible to use an > > out-of-band > > > > > > > data source, e.g. Kafka or Zookeeper, and then have your > > Connector > > > > > > > subscribe to changes. I've done something like this to enable > > dynamic > > > > > > > reconfiguration of Connectors from command-line tools and > > dashboards > > > > > > > without involving the Connect REST API at all. Moreover, I've > > done so in > > > > > > an > > > > > > > atomic, non-racy way. > > > > > > > > > > > > > > So I don't think PATCH is strictly necessary nor sufficient for > > atomic > > > > > > > partial updates. That said, it doesn't hurt and I'm happy to > > support the > > > > > > > KIP. > > > > > > > > > > > > > > Ryanne > > > > > > > > > > > > > > On Tue, Jun 25, 2019 at 12:15 PM Ivan Yurchenko < > > > > > > ivan0yurche...@gmail.com> > > > > > > > wrote: > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > Since Kafka 2.3 has just been release and more people may have > > time to > > > > > > > look > > > > > > > > at this now, I'd like to bump this discussion. > > > > > > > > Thanks. > > > > > > > > > > > > > > > > Ivan > > > > > > > > > > > > > > > > > > > > > > > > On Thu, 13 Jun 2019 at 17:20, Ivan Yurchenko < > > ivan0yurche...@gmail.com > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > Hello, > > > > > > > > > > > > > > > > > > I'd like to start the discussion of KIP-477: Add PATCH > > method for > > > > > > > > > connector config in Connect REST API. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-477%3A+Add+PATCH+method+for+connector+config+in+Connect+REST+API > > > > > > > > > > > > > > > > > > There is also a draft PR: > > https://github.com/apache/kafka/pull/6934. > > > > > > > > > > > > > > > > > > Thank you. > > > > > > > > > > > > > > > > > > Ivan > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
