kfaraz commented on code in PR #18142:
URL: https://github.com/apache/druid/pull/18142#discussion_r2146348472
##########
server/src/test/java/org/apache/druid/client/coordinator/CoordinatorClientImplTest.java:
##########
@@ -95,7 +98,9 @@ public void setup()
jsonMapper.setInjectableValues(
new InjectableValues.Std(ImmutableMap.of(
DataSegment.PruneSpecsHolder.class.getName(),
- DataSegment.PruneSpecsHolder.DEFAULT)));
+ DataSegment.PruneSpecsHolder.DEFAULT
+ )));
Review Comment:
```suggestion
DataSegment.PruneSpecsHolder.DEFAULT
))
);
```
##########
server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java:
##########
@@ -125,19 +113,19 @@ public class LookupReferencesManager implements
LookupExtractorFactoryContainerP
public LookupReferencesManager(
LookupConfig lookupConfig,
@Json ObjectMapper objectMapper,
- @Coordinator DruidLeaderClient druidLeaderClient,
- LookupListeningAnnouncerConfig lookupListeningAnnouncerConfig
+ LookupListeningAnnouncerConfig lookupListeningAnnouncerConfig,
+ CoordinatorClient coordinatorClient
Review Comment:
Nit: sticking to the older order of args will make the diff smaller. Same
comment for the other constructor.
```suggestion
CoordinatorClient coordinatorClient,
LookupListeningAnnouncerConfig lookupListeningAnnouncerConfig
```
##########
server/src/main/java/org/apache/druid/client/coordinator/CoordinatorClient.java:
##########
@@ -94,4 +96,13 @@ public interface CoordinatorClient
* API: {@code GET /druid/coordinator/v1/config}
*/
ListenableFuture<CoordinatorDynamicConfig> getCoordinatorDynamicConfig();
+
+ /**
+ * Gets the lookup configuration for a tier
+ * <p>
+ * API: {@code GET /druid/coordinator/v1/lookups/config/<tier>}
+ *
+ * @param tier The name of the tier for which the lookup
configuration is to be fetched.
+ */
+ ListenableFuture<Map<String, LookupExtractorFactoryContainer>>
fetchLookupForTier(String tier);
Review Comment:
```suggestion
ListenableFuture<Map<String, LookupExtractorFactoryContainer>>
fetchLookupsForTier(String tier);
```
##########
server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java:
##########
@@ -286,7 +273,11 @@ public void add(String lookupName,
LookupExtractorFactoryContainer lookupExtract
if (lookupLoadingSpec.getMode() == LookupLoadingSpec.Mode.NONE ||
(lookupLoadingSpec.getMode() == LookupLoadingSpec.Mode.ONLY_REQUIRED
&& !lookupLoadingSpec.getLookupsToLoad().contains(lookupName))) {
- LOG.info("Skipping notice to add lookup [%s] since current lookup
loading mode [%s] does not allow it.", lookupName, lookupLoadingSpec.getMode());
+ LOG.info(
+ "Skipping notice to add lookup [%s] since current lookup loading
mode [%s] does not allow it.",
Review Comment:
```suggestion
"Skipping notice to add lookup[%s] since current lookup loading
mode[%s] does not allow it.",
```
##########
server/src/test/java/org/apache/druid/client/coordinator/CoordinatorClientImplTest.java:
##########
@@ -450,4 +464,34 @@ public void test_getCoordinatorDynamicConfig() throws
Exception
coordinatorClient.getCoordinatorDynamicConfig().get()
);
}
+
+ @Test
+ public void test_fetchLookupForTier_detailedDisabled() throws Exception
+ {
+ LookupExtractorFactory lookupData = new MapLookupExtractorFactory(
+ Map.of(
+ "77483", "United States",
+ "77484", "India"
+ ), true
+ );
+ LookupExtractorFactoryContainer lookupDataContainer = new
LookupExtractorFactoryContainer("v0", lookupData);
+ Map<String, LookupExtractorFactoryContainer> lookups = Map.of(
+ "country_code", lookupDataContainer
+ );
+
+ serviceClient.expectAndRespond(
+ new RequestBuilder(
+ HttpMethod.GET,
+
"/druid/coordinator/v1/lookups/config/country_code?fetchDetails=true"
Review Comment:
query param name:
```suggestion
"/druid/coordinator/v1/lookups/config/country_code?detailed=true"
```
should the test name be `detailedEnabled`?
##########
server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java:
##########
@@ -476,38 +467,9 @@ private List<LookupBean>
getLookupListFromCoordinator(String tier)
@Nullable
private Map<String, LookupExtractorFactoryContainer>
tryGetLookupListFromCoordinator(String tier)
- throws IOException, InterruptedException
+ throws InterruptedException, ExecutionException
{
- final StringFullResponseHolder response = fetchLookupsForTier(tier);
- if (response.getStatus().equals(HttpResponseStatus.NOT_FOUND)) {
- LOG.warn("No lookups found for tier [%s], response [%s]", tier,
response);
- return null;
- } else if (!response.getStatus().equals(HttpResponseStatus.OK)) {
- throw new IOE(
- "Error while fetching lookup code from Coordinator with status[%s]
and content[%s]",
- response.getStatus(),
- response.getContent()
- );
- }
-
- // Older version of getSpecificTier returns a list of lookup names.
- // Lookup loading is performed via snapshot if older version is present.
- // This check is only for backward compatibility and should be removed in
a future release
- if (response.getContent().startsWith("[")) {
- LOG.info(
- "Failed to retrieve lookup information from coordinator, " +
- "because coordinator appears to be running on older Druid version. "
+
- "Attempting to load lookups using snapshot instead"
- );
- return null;
- } else {
- Map<String, Object> lookupNameToGenericConfig =
- jsonMapper.readValue(response.getContent(),
LOOKUPS_ALL_GENERIC_REFERENCE);
- return LookupUtils.tryConvertObjectMapToLookupConfigMap(
- lookupNameToGenericConfig,
- jsonMapper
- );
- }
+ return coordinatorClient.fetchLookupForTier(tier).get();
Review Comment:
```suggestion
return
FutureUtils.getUnchecked(coordinatorClient.fetchLookupForTier(tier), true);
```
##########
server/src/main/java/org/apache/druid/client/coordinator/CoordinatorClientImpl.java:
##########
@@ -239,4 +243,36 @@ public ListenableFuture<CoordinatorDynamicConfig>
getCoordinatorDynamicConfig()
)
);
}
+
+ @Override
+ public ListenableFuture<Map<String, LookupExtractorFactoryContainer>>
fetchLookupForTier(
+ String tier
+ )
+ {
+ final String path = StringUtils.format(
+ "/druid/coordinator/v1/lookups/config/%s?fetchDetails=true",
Review Comment:
Query param name, I think this is causing ITs to fail.
```suggestion
"/druid/coordinator/v1/lookups/config/%s?detailed=true",
```
##########
server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java:
##########
@@ -844,7 +803,10 @@ public void testCoordinatorLoadNoLookups() throws Exception
public void testCoordinatorLoadSubsetOfLookups() throws Exception
{
Map<String, LookupExtractorFactoryContainer> lookupMap =
-
getLookupMapForSelectiveLoadingOfLookups(LookupLoadingSpec.loadOnly(ImmutableSet.of("testLookup1",
"testLookup2")));
+
getLookupMapForSelectiveLoadingOfLookups(LookupLoadingSpec.loadOnly(ImmutableSet.of(
+ "testLookup1",
+ "testLookup2"
+ )));
Review Comment:
Nit: Preferred formatting as it takes fewer lines and is more readable.
```suggestion
getLookupMapForSelectiveLoadingOfLookups(
LookupLoadingSpec.loadOnly(Set.of("testLookup1", "testLookup2"))
);
```
##########
server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java:
##########
@@ -973,26 +923,23 @@ public boolean getEnableLookupSyncOnStartup()
LookupReferencesManager lookupReferencesManager = new
LookupReferencesManager(
lookupConfig,
mapper,
- druidLeaderClient,
- config
+ config,
+ coordinatorClient
);
- Map<String, Object> lookupMap = new HashMap<>();
+ Map<String, LookupExtractorFactoryContainer> lookupMap = new HashMap<>();
lookupMap.put("testMockForDisableLookupSync", container);
- String strResult = mapper.writeValueAsString(lookupMap);
- Request request = new Request(HttpMethod.GET, new
URL("http://localhost:1234/xx";));
EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes();
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);
+ ListenableFuture<Map<String, LookupExtractorFactoryContainer>> future =
new AbstractFuture<>()
+ {
+ @Override
+ public Map<String, LookupExtractorFactoryContainer> get()
+ {
+ return lookupMap;
+ }
+ };
+
EasyMock.expect(coordinatorClient.fetchLookupForTier(LOOKUP_TIER)).andReturn(future);
Review Comment:
Please use similar change in other places.
```suggestion
EasyMock.expect(coordinatorClient.fetchLookupForTier(LOOKUP_TIER)).andReturn(Futures.immediateFuture(lookupMap));
```
##########
server/src/test/java/org/apache/druid/client/coordinator/CoordinatorClientImplTest.java:
##########
@@ -450,4 +464,34 @@ public void test_getCoordinatorDynamicConfig() throws
Exception
coordinatorClient.getCoordinatorDynamicConfig().get()
);
}
+
+ @Test
+ public void test_fetchLookupForTier_detailedDisabled() throws Exception
+ {
+ LookupExtractorFactory lookupData = new MapLookupExtractorFactory(
+ Map.of(
+ "77483", "United States",
+ "77484", "India"
+ ), true
Review Comment:
Second arg on next line.
```suggestion
),
true
```
##########
server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java:
##########
@@ -476,38 +467,9 @@ private List<LookupBean>
getLookupListFromCoordinator(String tier)
@Nullable
private Map<String, LookupExtractorFactoryContainer>
tryGetLookupListFromCoordinator(String tier)
- throws IOException, InterruptedException
+ throws InterruptedException, ExecutionException
Review Comment:
Don't need the throws declaration if we use `FutureUtils.getUnchecked()`.
--
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]
