Thanks for the KIP Randall. It might not be obvious right away, but this is a great improvement when running Connect with multiple connectors or when debugging Connect, for instance in integration tests or system tests. KIP looks good to me overall, I have just a few comments below:
1. In the snippet of the config, by including an uncommented line, maybe it's not immediately clear that this line is an existing line in connect-log4j.properties and not an addition. Should this be mentioned in a separate code block or in a different way? 2. Currently when adding the taskId to a connector or connector task, we precede it with a dash (-). I feel this addition would make it easier to parse the taskId visually as well as with parsing tools (for example I can use `-` as a delimiter easier than 'k' or even a multi-character delimiter such as 'task'). Do you think it's an overhead? 3. I think a specific mention to the convention used on the base-index for taskIds would be useful. For example, taskId equal to zero in most cases represents the (source/sink) connector, while taskId > 0 corresponds to (source/sink) tasks. By reading the KIP, I assume that task0 (or task-0 if you agree with my previous comment) will be the connector. Is this the case? 4. Should the example include a snippet for taskId > 0? 5. Do you intend to enable connector MDC in tests? In this case how would you distinguish between multiple workers in integrations tests? Do you intend to address this here, in the PR corresponding to this KIP or follow up later? 6. Related to my previous comment (5). The property 'client.id' is already used to identify a Connect worker. Have you thought of offering the ability to identify the worker MDC if this property is set, keeping at the same time with MDC the ability to not include it as identifier? Finally, I'd suggest removing text placeholders. I think it will be easier to follow the KIP. Currently I see placeholder text below Contents and in Rejected alternatives. And my least favorite section, but I can't help it :) Typos/grammar: to understand what (is) happening within the worker, Kafka Connect use(s) ... quotes maybe not needed, text already verbatim in `config/connect-log4j.properties` Best, Konstantine On Tue, Apr 2, 2019 at 4:20 PM Randall Hauch <rha...@gmail.com> wrote: > I've been working on https://github.com/apache/kafka/pull/5743 for a > while, > but there were a number of comment, suggestions, and mild concerns on the > PR. One of those comments was that maybe changing the Connect log content > in this way probably warrants a KIP. So here it is: > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-449%3A+Add+connector+contexts+to+Connect+worker+logs > > I've also updated my PR reflect the KIP. Please reply with comments and/or > feedback. > > Best regards, > > Randall >
