I updated the KIP accordingly. Tried to come up with more generic terms in the Connect code instead of referring to "flow" anywhere. Daniel
Dániel Urbán <urb.dani...@gmail.com> ezt írta (időpont: 2023. jún. 5., H, 14:49): > 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 >> > >> >
