kfaraz commented on code in PR #16328:
URL: https://github.com/apache/druid/pull/16328#discussion_r1578700690
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/Task.java:
##########
@@ -331,4 +333,10 @@ static TaskInfo<TaskIdentifier, TaskStatus>
toTaskIdentifierInfo(TaskInfo<Task,
taskInfo.getTask().getMetadata()
);
}
+
+ @Nullable
+ default List<String> getLookupsToLoad()
Review Comment:
Please add a javadoc for this.
##########
server/src/test/java/org/apache/druid/query/lookup/LookupListeningAnnouncerConfigTest.java:
##########
@@ -127,6 +137,18 @@ public void testDatasourceInjection()
Assert.assertEquals("some_datasource", config.getLookupTier());
}
+ @Test
+ public void testLookupsToLoadInjection()
+ {
+ ArrayList<String> lookupsToLoad = new ArrayList<>();
+ lookupsToLoad.add("lookupName1");
+ lookupsToLoad.add("lookupName2");
Review Comment:
```suggestion
final List<String> lookupsToLoad = Arrays.asList("lookupName1",
"lookupName2");
```
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -339,4 +339,10 @@ private NavigableMap<DateTime, List<TaskLock>>
getNonRevokedTaskLockMap(TaskActi
);
return taskLockMap;
}
+
+ @Override
+ public List<String> getLookupsToLoad()
+ {
+ return new ArrayList<>();
Review Comment:
```suggestion
return Collections.emptyList();
```
##########
server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java:
##########
@@ -373,11 +374,28 @@ private void takeSnapshot(Map<String,
LookupExtractorFactoryContainer> lookupMap
}
}
- private void loadAllLookupsAndInitStateRef()
+ /**
+ * Load a set of lookups.
+ * @param lookupsToLoad List of lookup names to load. Pass null to load all
lookups.
+ */
+ private void loadLookupsAndInitStateRef(List<String> lookupsToLoad)
Review Comment:
Argument not needed as the config is directly accessible from this method
itself.
```suggestion
private void loadLookupsAndInitStateRef()
```
##########
server/src/test/java/org/apache/druid/query/lookup/LookupListeningAnnouncerConfigTest.java:
##########
@@ -57,6 +60,13 @@ public void configure(Binder binder)
binder
.bind(Key.get(String.class,
Names.named(DataSourceTaskIdHolder.DATA_SOURCE_BINDING)))
.toInstance("some_datasource");
+
+ ArrayList<String> lookupsToLoad = new ArrayList<>();
+ lookupsToLoad.add("lookupName1");
+ lookupsToLoad.add("lookupName2");
Review Comment:
```suggestion
final List<String> lookupsToLoad =
Arrays.asList("lookupName1", "lookupName2");
```
##########
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(
Review Comment:
Map can be in a single line without compromising readability.
##########
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:
Since the assertions are different, it would be better to break these cases
up into separate test methods. You can use private methods to contain the
common code used in the 3 tests.
##########
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
+ },
Review Comment:
this can be in a single line without compromising readability, same for the
other elements in this array.
##########
server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java:
##########
@@ -373,11 +374,28 @@ private void takeSnapshot(Map<String,
LookupExtractorFactoryContainer> lookupMap
}
}
- private void loadAllLookupsAndInitStateRef()
+ /**
+ * Load a set of lookups.
+ * @param lookupsToLoad List of lookup names to load. Pass null to load all
lookups.
+ */
+ private void loadLookupsAndInitStateRef(List<String> lookupsToLoad)
{
List<LookupBean> lookupBeanList = getLookupsList();
- if (lookupBeanList != null) {
- startLookups(lookupBeanList);
+
+ List<LookupBean> lookupBeansToLoad;
+ if (lookupsToLoad != null) {
+ lookupBeansToLoad = new ArrayList<>();
+ if (lookupBeanList != null) {
+ lookupBeansToLoad = lookupBeanList.stream()
+ .filter(lookupBean ->
lookupsToLoad.contains(lookupBean.getName()))
+ .collect(Collectors.toList());
+ }
+ } else {
+ lookupBeansToLoad = lookupBeanList;
+ }
+
+ if (lookupBeansToLoad != null) {
+ startLookups(lookupBeansToLoad);
Review Comment:
This can be simplified:
```suggestion
final List<String> lookupsToLoad =
lookupListeningAnnouncerConfig.getLookupsToLoad();
final boolean loadAllLookups = lookupsToLoad == null;
if (lookupBeanList != null) {
lookupBeanList = lookupBeanList.stream()
.filter(lookupBean -> loadAllLookups ||
lookupsToLoad.contains(lookupBean.getName()))
.collect(Collectors.toList());
startLookups(lookupBeansToLoad);
```
--
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]
