github-code-scanning[bot] commented on code in PR #14092:
URL: https://github.com/apache/druid/pull/14092#discussion_r1167295867
##########
server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java:
##########
@@ -211,6 +222,81 @@
Assert.assertEquals("hello", druidLeaderClient.go(request).getContent());
}
+ @Test
+ public void testNon200ResponseFromServerAndCacheRefresh() throws Exception
+ {
+ DruidNodeDiscovery druidNodeDiscovery =
EasyMock.createMock(DruidNodeDiscovery.class);
+ // Should be called twice. Second time is when we refresh the cache since
we get 503 in the first request
+
EasyMock.expect(druidNodeDiscovery.getAllNodes()).andReturn(ImmutableList.of(discoveryDruidNode)).times(2);
+
+ DruidNodeDiscoveryProvider druidNodeDiscoveryProvider =
EasyMock.createMock(DruidNodeDiscoveryProvider.class);
+
EasyMock.expect(druidNodeDiscoveryProvider.getForNodeRole(NodeRole.PEON)).andReturn(druidNodeDiscovery).anyTimes();
+
+ ListenableFutureTask task =
EasyMock.createMock(ListenableFutureTask.class);
+ EasyMock.expect(task.get()).andReturn(new StringFullResponseHolder(new
DefaultHttpResponse(
+ HttpVersion.HTTP_1_1,
+ HttpResponseStatus.SERVICE_UNAVAILABLE
+ ), Charset.defaultCharset()));
+ EasyMock.replay(druidNodeDiscovery, druidNodeDiscoveryProvider, task);
+
+ HttpClient httpClient = Mockito.spy(this.httpClient);
Review Comment:
## Possible confusion of local and field
Potentially confusing name: method
[testNon200ResponseFromServerAndCacheRefresh](1) also refers to field
[httpClient](2) (as this.httpClient).
[Show more
details](https://github.com/apache/druid/security/code-scanning/4815)
##########
server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java:
##########
@@ -211,6 +222,81 @@
Assert.assertEquals("hello", druidLeaderClient.go(request).getContent());
}
+ @Test
+ public void testNon200ResponseFromServerAndCacheRefresh() throws Exception
+ {
+ DruidNodeDiscovery druidNodeDiscovery =
EasyMock.createMock(DruidNodeDiscovery.class);
+ // Should be called twice. Second time is when we refresh the cache since
we get 503 in the first request
+
EasyMock.expect(druidNodeDiscovery.getAllNodes()).andReturn(ImmutableList.of(discoveryDruidNode)).times(2);
+
+ DruidNodeDiscoveryProvider druidNodeDiscoveryProvider =
EasyMock.createMock(DruidNodeDiscoveryProvider.class);
+
EasyMock.expect(druidNodeDiscoveryProvider.getForNodeRole(NodeRole.PEON)).andReturn(druidNodeDiscovery).anyTimes();
+
+ ListenableFutureTask task =
EasyMock.createMock(ListenableFutureTask.class);
+ EasyMock.expect(task.get()).andReturn(new StringFullResponseHolder(new
DefaultHttpResponse(
+ HttpVersion.HTTP_1_1,
+ HttpResponseStatus.SERVICE_UNAVAILABLE
+ ), Charset.defaultCharset()));
+ EasyMock.replay(druidNodeDiscovery, druidNodeDiscoveryProvider, task);
+
+ HttpClient httpClient = Mockito.spy(this.httpClient);
+ // Override behavior for the first call only
+
Mockito.doReturn(task).doCallRealMethod().when(httpClient).go(Mockito.any(),
Mockito.any());
+
+ DruidLeaderClient druidLeaderClient = new DruidLeaderClient(
+ httpClient,
+ druidNodeDiscoveryProvider,
+ NodeRole.PEON,
+ "/simple/leader"
+ );
+ druidLeaderClient.start();
+
+ Request request = druidLeaderClient.makeRequest(HttpMethod.POST,
"/simple/direct");
+ request.setContent("hello".getBytes(StandardCharsets.UTF_8));
+ Assert.assertEquals("hello", druidLeaderClient.go(request).getContent());
+ EasyMock.verify(druidNodeDiscovery);
+ }
+
+ @Test
+ public void testNon200ResponseFromServerNoCacheRefresh() throws Exception
+ {
+ DruidNodeDiscovery druidNodeDiscovery =
EasyMock.createMock(DruidNodeDiscovery.class);
+ // Should be called only once, since we want to handle 503 ourselves and
don't want cache refresh
+
EasyMock.expect(druidNodeDiscovery.getAllNodes()).andReturn(ImmutableList.of(discoveryDruidNode)).times(1);
+
+ DruidNodeDiscoveryProvider druidNodeDiscoveryProvider =
EasyMock.createMock(DruidNodeDiscoveryProvider.class);
+
EasyMock.expect(druidNodeDiscoveryProvider.getForNodeRole(NodeRole.PEON)).andReturn(druidNodeDiscovery).anyTimes();
+
+ ListenableFutureTask task =
EasyMock.createMock(ListenableFutureTask.class);
+ EasyMock.expect(task.get()).andReturn(new StringFullResponseHolder(new
DefaultHttpResponse(
+ HttpVersion.HTTP_1_1,
+ HttpResponseStatus.SERVICE_UNAVAILABLE
+ ), Charset.defaultCharset()));
+ EasyMock.replay(druidNodeDiscovery, druidNodeDiscoveryProvider, task);
+
+ HttpClient httpClient = Mockito.spy(this.httpClient);
Review Comment:
## Possible confusion of local and field
Potentially confusing name: method
[testNon200ResponseFromServerNoCacheRefresh](1) also refers to field
[httpClient](2) (as this.httpClient).
[Show more
details](https://github.com/apache/druid/security/code-scanning/4816)
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
