KAFKA-14099 - Fix request logs in connect

[zigarn]

KAFKA-14099

Restore request logs in connect. RequestLogHandler seems to not work.

Committer Checklist (excluded from commit message)

• [ ] Verify design and implementation
• [ ] Verify test coverage and CI build status
• [ ] Verify documentation (including upgrade notes)

[vamossagar12]

thanks for the changes @zigarn . The changes look fine to me.. @C0urante can you also plz review? Thanks!

[zigarn]

Thanks @C0urante for the feedback. No problem for the issue in the first place, I guess not many people were impacted by this as nobody raised this issue.

I'm more concerned about why it's not working in the current way to do it. I wonder if the DefaultHandler is working either? From my understanding, looks like context and adminContext are working because they are explicitly started, but I tried to do the same with requestLogHandler with no luck.

I was wondering if there was a way to test this, so if you say there is with LogCaptureAppender, let's go for it!

[zigarn]

@C0urante In the meantime I added a test by copying the LogCaptureAppender from #10528.

Not the best with the while loop to wait for the log to be created in the completion phase. A more robust solution would be to create an handler that will do the test in the completion phase, but this will need to expose the RestServer.handlers. I'll see if something can work.

[zigarn]

Thanks for this thorough feedback @C0urante!

Fixed the checkstyle/import-control.xml, did revert too much after a lot of testing and importing new packages.

About the DefaultHandler, looks like it's currently not working anyway and didn't find a way to make it work... From my debugging, looks like the context handler is putting the request in HANDLED status and next handlers are skipped. Maybe an issue by mixing simple handler and context handlers? Maybe DefaultHandler should be set as an inner handler of the context handlers? I have to say that it's not easy to get into all this handler mechanism of Jetty.

I'll add the handling of autoclosable client and responses.

[zigarn]

@C0urante: what about using the existing scala implementation kafka.utils.LogCaptureAppender? I added a commit to try it and it works. May add some methods in scala implementation for easier use in java tests (instead of doing conversions in java test classes), could also benefit for #10528 in reverse? WDYT?

[C0urante]

@zigarn I think we try to avoid adding dependencies from the core module into non-broker modules with few exceptions (such as spinning up embedded Kafka clusters for integration tests), but I can ask. I did some research and it appears the Java variant was introduced a few months before the Scala one, so it's possible that we haven't made the switch yet just because nobody's proposed it.

[C0urante]

Apologies for the delay @zigarn, and thank you for sticking with this.

I'm still not sure we should be relying on the Scala LogCaptureAppender, but I don't want to ask you to put more work into this PR (you've already done plenty!), and I don't want to block this PR on someone else re-reviewing #10528.

What I'd like to do is merge this PR, then on #10528:

• Rebase onto the latest trunk
• Revert the changes to the Scala LogCaptureAppender introduced in this PR
• Update the RestServerTest to use the Java LogCaptureAppender
• Merge #10528 (it's already been approved and, although I was hoping we could get another look at it, it's technically acceptable to merge it now as-is)

Is that alright with you? And again, thank you for sticking with this PR, it really is a valuable improvement and I'd like to see it merged.

[zigarn]

@C0urante No problem. Your scenario is fine by me.