kfaraz commented on code in PR #16328:
URL: https://github.com/apache/druid/pull/16328#discussion_r1578991260
##########
server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java:
##########
@@ -765,6 +777,91 @@ public void testCoordinatorLookupSync() throws Exception
}
+ public static Object[]
parametersForTestCoordinatorSelectiveLoadingOfLookups()
+ {
+ return new Object[] {
+ // load all lookups
+ new Object[]{
+ null
+ },
+ // don't load any lookups
+ new Object[]{
+ Collections.emptyList()
+ },
+ // only load these lookups
+ new Object[]{
+ Arrays.asList("testLookup1", "testLookup2")
+ },
+ };
+ }
+
+ @Test
+ @Parameters
+ public void testCoordinatorSelectiveLoadingOfLookups(List<String>
lookupsToLoad) throws Exception
+ {
+ LookupExtractorFactoryContainer container1 = new
LookupExtractorFactoryContainer(
+ "0",
+ new MapLookupExtractorFactory(
+ ImmutableMap.of(
+ "key1",
+ "value1"
+ ), true
+ )
+ );
+
+ LookupExtractorFactoryContainer container2 = new
LookupExtractorFactoryContainer(
+ "0",
+ new MapLookupExtractorFactory(
+ ImmutableMap.of(
+ "key2",
+ "value2"
+ ), true
+ )
+ );
+
+ LookupExtractorFactoryContainer container3 = new
LookupExtractorFactoryContainer(
+ "0",
+ new MapLookupExtractorFactory(
+ ImmutableMap.of(
+ "key3",
+ "value3"
+ ), true
+ )
+ );
+ EasyMock.reset(config);
+ EasyMock.reset(druidLeaderClient);
+ Map<String, Object> lookupMap = new HashMap<>();
+ lookupMap.put("testLookup1", container1);
+ lookupMap.put("testLookup2", container2);
+ lookupMap.put("testLookup3", container3);
+ String strResult = mapper.writeValueAsString(lookupMap);
+ Request request = new Request(HttpMethod.GET, new
URL("http://localhost:1234/xx";));
+ EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER);
+ EasyMock.expect(config.getLookupsToLoad()).andReturn(lookupsToLoad);
+ EasyMock.replay(config);
+ EasyMock.expect(druidLeaderClient.makeRequest(
+ HttpMethod.GET,
+ "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true"
+ ))
+ .andReturn(request);
+ StringFullResponseHolder responseHolder = new StringFullResponseHolder(
+ newEmptyResponse(HttpResponseStatus.OK),
+ StandardCharsets.UTF_8
+ ).addChunk(strResult);
+ EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder);
+ EasyMock.replay(druidLeaderClient);
+
+ lookupReferencesManager.start();
+
+ for (String lookupName : lookupMap.keySet()) {
+ if (lookupsToLoad == null || lookupsToLoad.contains(lookupName)) {
+ Assert.assertEquals(Optional.of(lookupMap.get(lookupName)),
lookupReferencesManager.get(lookupName));
+ } else {
+
Assert.assertFalse(lookupReferencesManager.get(lookupName).isPresent());
Review Comment:
> I don't think this would be cleaner than the current approach of
parameterized test. Do you have any strict reason for breaking it up into
separate tests?
Yes, the current structure is not very preferable for someone reading the
test, especially because of the conditional assertions. Reading the test, we
don't know what we intend to verify until the very end.
Typically, the test name itself should be indicative of what we expect to
happen.
> Method for creating LookupExtractorFactoryContainer object, and returning
it to the individual tests for assertion purposes.
For performing the assertions, we only need `lookupReferencesManager` and
the `lookupMap`, right?
##########
server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java:
##########
@@ -38,19 +40,28 @@
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
import java.io.IOException;
import java.net.URL;
import java.nio.charset.StandardCharsets;
+import java.util.Arrays;
+import java.util.Collections;
import java.util.HashMap;
+import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
+@RunWith(JUnitParamsRunner.class)
public class LookupReferencesManagerTest
{
private static final String LOOKUP_TIER = "lookupTier";
+
+ // null indicates that all lookups need to be loaded.
Review Comment:
Nit: instead of this comment, you may just name the field that indicates
this. e.g. `ALL_LOOKUPS`.
--
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]
