C0urante commented on code in PR #12434:
URL: https://github.com/apache/kafka/pull/12434#discussion_r932668027
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/RestServer.java:
##########
@@ -275,14 +279,7 @@ public void initializeResources(Herder herder) {
configureHttpResponsHeaderFilter(context);
}
- RequestLogHandler requestLogHandler = new RequestLogHandler();
- Slf4jRequestLogWriter slf4jRequestLogWriter = new
Slf4jRequestLogWriter();
-
slf4jRequestLogWriter.setLoggerName(RestServer.class.getCanonicalName());
- CustomRequestLog requestLog = new
CustomRequestLog(slf4jRequestLogWriter, CustomRequestLog.EXTENDED_NCSA_FORMAT +
" %{ms}T");
- requestLogHandler.setRequestLog(requestLog);
-
contextHandlers.add(new DefaultHandler());
Review Comment:
I think we can remove this.
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/rest/RestServerTest.java:
##########
@@ -368,33 +414,26 @@ private void checkCustomizedHttpResponseHeaders(String
headerConfig, Map<String,
doReturn(Arrays.asList("a", "b")).when(herder).connectors();
server = new RestServer(workerConfig);
- try {
- server.initializeServer();
- server.initializeResources(herder);
- HttpRequest request = new HttpGet("/connectors");
- try (CloseableHttpClient httpClient = HttpClients.createMinimal())
{
- HttpHost httpHost = new
HttpHost(server.advertisedUrl().getHost(), server.advertisedUrl().getPort());
- try (CloseableHttpResponse response =
httpClient.execute(httpHost, request)) {
- Assert.assertEquals(200,
response.getStatusLine().getStatusCode());
- if (!headerConfig.isEmpty()) {
- expectedHeaders.forEach((k, v) ->
-
Assert.assertEquals(response.getFirstHeader(k).getValue(), v));
- } else {
-
Assert.assertNull(response.getFirstHeader("X-Frame-Options"));
- }
- }
- }
- } finally {
- server.stop();
- server = null;
+ server.initializeServer();
+ server.initializeResources(herder);
+ HttpRequest request = new HttpGet("/connectors");
+ httpClient = HttpClients.createMinimal();
+ HttpHost httpHost = new HttpHost(server.advertisedUrl().getHost(),
server.advertisedUrl().getPort());
+ response = httpClient.execute(httpHost, request);
+ Assert.assertEquals(200, response.getStatusLine().getStatusCode());
+ if (!headerConfig.isEmpty()) {
+ expectedHeaders.forEach((k, v) ->
+ Assert.assertEquals(response.getFirstHeader(k).getValue(),
v));
+ } else {
+ Assert.assertNull(response.getFirstHeader("X-Frame-Options"));
}
}
private String executeGet(String host, int port, String endpoint) throws
IOException {
HttpRequest request = new HttpGet(endpoint);
- CloseableHttpClient httpClient = HttpClients.createMinimal();
+ httpClient = HttpClients.createMinimal();
Review Comment:
I think we might have to use a try-with-resources block for the client and
response in this and `executePut` since those can be invoked multiple times per
test, in which case we'd end up overwriting the `httpClient` and `response`
fields without closing them.
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/rest/RestServerTest.java:
##########
@@ -199,12 +217,12 @@ public void checkCORSRequest(String corsDomain, String
origin, String expectedHe
HttpRequest request = new HttpGet("/connectors");
request.addHeader("Referer", origin + "/page");
request.addHeader("Origin", origin);
- CloseableHttpClient httpClient = HttpClients.createMinimal();
+ httpClient = HttpClients.createMinimal();
HttpHost httpHost = new HttpHost(
server.advertisedUrl().getHost(),
server.advertisedUrl().getPort()
);
- CloseableHttpResponse response = httpClient.execute(httpHost, request);
+ response = httpClient.execute(httpHost, request);
Review Comment:
We still have to close this `response` since it gets overwritten by a
different one before the test completes.
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/rest/RestServerTest.java:
##########
@@ -75,6 +79,20 @@ public void setUp() {
@After
public void tearDown() {
+ if (response != null) {
+ try {
+ response.close();
+ } catch (IOException e) {
+ e.printStackTrace();
+ }
Review Comment:
Can we remove the try/catch and add a `throws IOException` (or even just
`throws Exception`) declaration to the `tearDown` method?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
