Thanks, Chris and Jeremy.
I've fixed the errors that Chris mentioned. I agree with Jeremy that this
KIP is limited to the framework dealing with these contexts as part of its
logging mechanism, and they're not really for connectors to use. Connector
implementations that don't have additional threading will need to do
nothing to benefit from more useful logs. Connectors that *do* start
additional threads can generate a similar context for their threads, just
as long as they don't match the proposed context values -- otherwise the
distinct contexts will be indistinguishable in the logs.
I'd prefer to keep the current scope of the KIP. I don't believe there is
much benefit to exposing this to connector implementations, mostly because
any additional connector-level API requires connectors to become reliant
and deployable into a specific version of Connect.
And finally, I'm going to start a vote on this KIP since all of the raised
issues have been addressed.
Best regards,
Randall
On Tue, Apr 16, 2019 at 8:59 AM Jeremy Custenborder <jcustenbor...@gmail.com>
wrote:
> Chris is correct the examples are mixed but it's pretty easy to
> follow. From what I have gathered it looks like manipulation of the
> context would be handled by the framework and not necessarily for the
> connector developer. I'm not sure what benefit a connector developer
> would have to manipulate the connector context further. If we head
> down the path of allowing developers to extend the context, I would
> prefer the output format of %X{connector.context} to be key value.
> Something like "connector=myconnector task=0"
>
> The state of the corresponding pull request looks really good as is. I
> would be fine with merging it as is or expanding it to write the
> context as key value.
>
> On Mon, Apr 15, 2019 at 12:55 PM Chris Egerton <chr...@confluent.io>
> wrote:
> >
> > Hi Randall,
> >
> > Thanks for the KIP. Debugging Connect workers definitely becomes harder
> as
> > the number of connectors and tasks increases, and your proposal would
> > simplify the process of sifting through logs and finding relevant
> > information faster and more accurately.
> >
> > I have a couple small comments:
> >
> > First--I believe the example snippet in your KIP under the "Public
> > Interfaces" header is inaccurate:
> > `[my-connector|worker]` - used on log messages where the Connect worker
> is
> > validating the configuration for or starting/stopping the
> > "local-file-source" connector via the SourceConnector / SinkConnector
> > implementation methods.
> > `[my-connector|task-0]` - used on log messages where the Connect worker
> is
> > executing task 0 of the "local-file-source" connector, including calling
> > any of the SourceTask / SinkTask implementation methods, processing the
> > messages for/from the task, and calling the task's producer/consumer.
> > `[my-connector|task-0|offsets]` - used on log messages where the Connect
> > worker is committing source offsets for task 0 of the "local-file-source"
> > connector.
> > The sample contexts mention "my-connector" but their explanations say
> that
> > they correspond to "local-file-source"; shouldn't the two align?
> >
> > Second--I'm unclear on whether we expect (or want to encourage)
> developers
> > to manipulate the "connector.context" MDC key themselves, from with
> > connectors, transforms, etc. If we want to encourage this (in order to
> make
> > debugging even easier, which would align with the intent behind this
> KIP),
> > we may want to expose the LoggingContext class in the Connect API package
> > and expand on it so that users can set the context themselves. This would
> > be especially helpful in connectors with multithreaded logic. However, if
> > that would expand the scope of this KIP too much I think we could afford
> to
> > address that later.
> >
> > Cheers,
> >
> > Chris
>
