niket-goel commented on PR #12206: URL: https://github.com/apache/kafka/pull/12206#issuecomment-1138153211
Thanks for review @dengziming . A few comments: > Firstly, we can add a test in PlaintextAdminIntegrationTest for this change I looked at the file and the tests there look similar to the what was added to `KraftClusterTest`. Could you check if the intent was to run an integration test like that? We can always increase the checks that test does. > the second one is less important, it's recommended to use AdminApiHandler instead of using runnable.call directly, you can refer to FenceProducersHandler.java as a simple example. I looked at the class and it looks like a wrapper for the runnable call. Do you mind if we make that improvement in a subsequent change (just trying to get this PR under control :) ) > I wonder if we can send it to controllerSockerServer directly? for example, we set bootstrap.server=localhost:9093, we should prevent a client from doing this. A couple of things here -- This code existed before this change (I just moved it). I would love to improve it though. I just did not understand what exactly your comment meant. Could you please elaborate? Apart from the above questions I think I have addressed all the other concerns everyone raised in the latest version. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
