Hi Chris,

Thank you for your comments!

I agree that the toString based logging is not ideal, and I believe all
occurrences are within a proper logging context, so they can be ignored.
If thread names can be changed unconditionally, I agree, using a new MDC
key is the ideal solution.

Will update the KIP accordingly.

Thanks,
Daniel

Chris Egerton <chr...@aiven.io.invalid> ezt írta (időpont: 2023. jún. 2.,
P, 22:23):

> Hi Daniel,
>
> Are there any cases of Object::toString being used by internal classes for
> logging context that can't be covered by MDC contexts? For example,
> anything logged by WorkerSinkTask (or any WorkerTask subclass) already has
> the MDC set for the task [1]. Since the Object::toString-prefixed style of
> logging is a bit obsolete after the introduction of MDC contexts it'd feel
> a little strange to continue trying to accommodate it, especially if the
> changes from this KIP are going to be opt-in regardless.
>
> As far as thread names go: unlike log statements, I don't believe changing
> them requires a KIP, and would be fine with merging a PR for that without
> worrying about compatibility.
>
> With that in mind, I'm still wondering if we can add a separate MDC key for
> MM2 replication flows (perhaps "mirror.maker.flow") and unconditionally add
> that to the MDC contexts for every thread that gets spun up by each
> DistributedHerder instance that MM2 creates. This would be different from
> the draft PR and KIP, which involve altering the content of the existing
> "connector.context" MDC key. Users would opt in to the change by altering
> their Log4j config instead of altering their MM2 config, which matches
> precedent set with the introduction of the "connector.context" key.
>
> Thoughts?
>
> [1] -
>
> https://github.com/apache/kafka/blob/146a6976aed0d9f90c70b6f21dca8b887cc34e71/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerTask.java#L255
>
> Cheers,
>
> Chris
>
> On Tue, May 23, 2023 at 11:36 AM Dániel Urbán <urb.dani...@gmail.com>
> wrote:
>
> > Hi Chris,
> >
> > Thank you for your comments!
> > 1. This approach is OK for me. I thought that the "sample" configs in the
> > repo do not fall into the same category as the default of a new config.
> > Adding a commented line instead should be ok, and the future opt-out
> change
> > sounds good to me.
> >
> > 2. Besides the MDC, there are 2 other places where the flow context
> > information could be added:
> > - Some of the Connect internal classes use their .toString methods when
> > logging (e.g. WorkerSinkTask has a line like this: log.info("{}
> Executing
> > sink task", this);). These toString implementations contain the connector
> > name, and nothing else, so in MM2 dedicated mode, adding the flow would
> > make these lines unique.
> > - Connect worker thread names. Currently, they contain the connector
> > name/task ID, but to make them unique, the flow should be added in MM2
> > distributed mode.
> > In my draft PR, I changed both of these, but for the sake of backward
> > compatibility, I made the new toString/thread name dependent on the new,
> > suggested configuration flag.
> > If we go with a new MDC key, but we still want to change the toString
> > methods and the thread names, we still might need an extra flag to turn
> on
> > the new behavior.
> > AFAIK the toString calls are all made inside a proper logging context, so
> > changing the toString methods don't add much value. I think that the
> thread
> > name changes are useful, giving more context for log lines written
> outside
> > of a log context.
> >
> > In short, I think that MDC + thread name changes are necessary to make
> MM2
> > dedicated logs useful for diagnostics. If we keep both, then maybe
> having a
> > single config to control both at the same time is better than having a
> new
> > MDC key (configured separately in the log pattern) and a config flag (set
> > separately in the properties file) for the thread name change.
> >
> > Thanks,
> > Daniel
> >
>

Reply via email to