Updated the KIP with a few example log lines before/after the change. Daniel
Dániel Urbán <urb.dani...@gmail.com> ezt írta (időpont: 2023. jún. 7., Sze, 13:59): > Hi Chris, > > Thank you for your comments! I updated the KIP. I still need to add the > example before/after log lines, will do that soon, but I addressed all the > other points. > 1. Added more details on thread renaming under Public Interfaces, removed > pseudo code. > 2. Removed the stale header - originally, client.id related changes were > part of the KIP, and I failed removing all leftovers of that version. > 3. Threads listed under Public Interfaces with current/proposed names. > 4. Added a comment in the connect-log4j.properties, similar to the one > introduced in KIP-449. We don't have a dedicated MM2 log4j config, not sure > if we should introduce it here. > 5. Clarified testing section - I think thread names should not be tested > (they never were), but testing will focus on the new MDC context value. > > Thanks, > Daniel > > Chris Egerton <chr...@aiven.io.invalid> ezt írta (időpont: 2023. jún. 5., > H, 16:46): > >> Hi Daniel, >> >> Thanks for the updates! A few more thoughts: >> >> 1. The "Proposed Changes" section seems a bit like a pseudocode >> implementation of the KIP. We don't really need this level of detail; most >> of the time, we're just looking for implementation details that don't >> directly affect the user-facing changes proposed in the "Public >> Interfaces" >> section but are worth mentioning because they (1) demonstrate how the >> user-facing changes are made possible, (2) indirectly affect user-facing >> behavior, or (3) go into more detail (like providing examples) about the >> user-facing behavior. For this KIP, I think examples of user-facing >> behavior (like before/after of thread names and log messages) and possibly >> an official description of the scope of the changes (which threads are >> going to be renamed and/or include the new MDC key, and which aren't?) are >> all that we'd really need in this section; everything else is fairly clear >> IMO. FWIW, the reason we want to discourage going into too much detail >> with >> KIPs is that it can quickly devolve into code review over mailing list, >> which can hold KIPs up for longer than necessary when the core design >> changes they contain are already basically accepted by everyone. >> >> 2. The "MM2 distributed mode client.id and log change" header seems like >> it >> may no longer be accurate; the contents do not mention any changes to >> client IDs. I might be missing something though; please correct me if I >> am. >> >> 3. Can you provide some before/after examples of what thread names and log >> messages will look like? I'm wondering about the thread that the >> distributed herder runs on, threads for connectors and tasks, and threads >> for polling internal topics (which we currently manage with the >> KafkaBasedLog class). It's fine if some of these are unchanged, I just >> want >> to better understand the scope of the proposed changes and get an idea of >> how they may appear to users. >> >> 4. There's no mention of changes to the default Log4j config files that we >> ship. Is this intentional? I feel like we need some way for users to >> easily >> discover this feature; if we're not going to touch our default Log4j >> config >> files, is there another way that we can expect users to find out about the >> new MDC key? >> >> 5. RE the "Test Plan" section: can you provide a little more detail of >> which cases we'll be covering with the proposed unit tests? Will we be >> verifying that the MDC context is set in various places? If so, which >> places? And the same with thread names? (There doesn't have to be a ton of >> detail, but a little more than "unit tests" would be nice 😄) >> >> Cheers, >> >> Chris >> >> On Mon, Jun 5, 2023 at 9:45 AM Dániel Urbán <urb.dani...@gmail.com> >> wrote: >> >> > 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 >> > >> > >> > >> >> > > >> > >> >
