> If no modifications to a logging namespace have
> been made, won't the namespace itself be omitted
> from the response? It looks like we currently only
> return loggers that have non-null log levels in the
*> **GET /admin/loggers* endpoint.
This can be ignored - I didn't account for the fact that at worker startup
we'll still have loggers with non-null log levels - the root logger and any
other named loggers which have explicit log levels configured in the log4j
properties.
On Mon, Sep 4, 2023 at 12:00 PM Yash Mayya <yash.ma...@gmail.com> wrote:
> Hi Chris,
>
> Thanks for the KIP, this looks like a really useful addition to Kafka
> Connect's log level REST APIs! I have a few questions and comments:
>
> > If no modifications to the namespace have
> > been made since the worker was started,
> > they will be null
>
> If no modifications to a logging namespace have been made, won't the
> namespace itself be omitted from the response? It looks like we currently
> only return loggers that have non-null log levels in the *GET
> /admin/loggers* endpoint.
>
> > Last modified timestamp
>
> From the proposed changes section, it isn't very clear to me how we'll be
> tracking this last modified timestamp to be returned in responses for the *GET
> /admin/loggers* and *GET /admin/loggers/{logger}* endpoints. Could you
> please elaborate on this? Also, will we track the last modified timestamp
> even for worker scoped modifications where we won't write any records to
> the config topic and the requests will essentially be processed
> synchronously?
>
> > Record values will have the following format, where ${level} is the new
> logging level for the namespace:
>
> In the current synchronous implementation for the *PUT
> /admin/loggers/{logger} *endpoint, we return a 404 error if the level is
> invalid (i.e. not one of TRACE, DEBUG, WARN etc.). Since the new cluster
> scoped variant of the endpoint will be asynchronous, can we also add a
> validation to synchronously surface erroneous log levels to users?
>
> > Workers that have not yet completed startup
> > will ignore these records, including if the worker
> > reads one during the read-to-end of the config
> > topic that all workers perform during startup.
>
> I'm curious to know what the rationale here is? In KIP-745, the stated
> reasoning behind ignoring restart requests during worker startup was that
> the worker will anyway be starting all connectors and tasks assigned to it
> so the restart request is essentially meaningless. With the log level API
> however, wouldn't it make more sense to apply any cluster scoped
> modifications to new workers in the cluster too? This also applies to any
> workers that are restarted soon after a request is made to *PUT
> /admin/loggers/{logger}?scope=cluster *on another worker. Maybe we could
> stack up all the cluster scoped log level modification requests during the
> config topic read at worker startup and apply the latest ones per namespace
> (assuming older records haven't already been compacted) after we've
> finished reading to the end of the config topic?
>
> > if you're debugging the distributed herder, you
> > need all the help you can get
>
> 😂
>
> As an aside, thanks for the impressively thorough testing plan in the KIP!
>
>
> Hi Ashwin,
>
> > isn't it better to forward the requests to the
> > leader in the initial implementation ?
>
> Would there be any downside to only introducing leader forwarding for
> connector/task specific scope types in the future (rather than introducing
> it at the outset here where it isn't strictly required)?
>
> > I would also recommend an additional system
> > test for Standalone herder to ensure that the
> > new scope parameter is honored and the response
> > contains the last modified time.
>
> Can't this be sufficiently covered using unit and / or integration tests?
> System tests are fairly expensive to run in terms of overall test runtime
> and they are also not run on every PR or commit to trunk / feature branches
> (unlike unit tests and integration tests).
>
> Thanks,
> Yash
>
> On Sat, Sep 2, 2023 at 5:12 AM Chris Egerton <chr...@aiven.io.invalid>
> wrote:
>
>> Hi all,
>>
>> Can't imagine a worse time to publish a new KIP (it's late on a Friday and
>> we're in the middle of the 3.6.0 release), but I wanted to put forth
>> KIP-976 for discussion:
>>
>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-976%3A+Cluster-wide+dynamic+log+adjustment+for+Kafka+Connect
>>
>> TL;DR: The API to dynamically adjust log levels at runtime with Connect is
>> great, and I'd like to augment it with support to adjust log levels for
>> every worker in the cluster (instead of just the worker that receives the
>> REST request).
>>
>> I look forward to everyone's thoughts, but expect that this will probably
>> take a bump or two once the dust has settled on 3.6.0. Huge thanks to
>> everyone that's contributed to that release so far, especially our release
>> manager Satish!
>>
>> Cheers,
>>
>> Chris
>>
>
