You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "uds5501 (via GitHub)" <gi...@apache.org> on 2025/06/13 20:11:50 UTC
[PR] Use CoordinatorClient for fetching tier lookup (druid)
uds5501 opened a new pull request, #18142:
URL: https://github.com/apache/druid/pull/18142
### Description
This PR is part of the effort to phase out `DruidLeaderClient` and utilize `CoordinatorClient` to interact with Coordinators. This particular PR focuses on
- Implementing interface `fetchLookupForTier` for Coordinator Client.
- Replacing `DruidLeaderClient` in `LookupReferencesManager` with `CoordinatorClient`.
<hr>
##### Key changed/added classes in this PR
* `CoordinatorClientImpl.fetchLookupForTier()`
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [x] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [x] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Use CoordinatorClient for fetching tier lookup (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18142:
URL: https://github.com/apache/druid/pull/18142#discussion_r2147409913
##########
server/src/test/java/org/apache/druid/client/coordinator/CoordinatorClientImplTest.java:
##########
@@ -450,4 +465,35 @@ public void test_getCoordinatorDynamicConfig() throws Exception
coordinatorClient.getCoordinatorDynamicConfig().get()
);
}
+
+ @Test
+ public void test_fetchLookupsForTier_detailedEnabled() 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?detailed=true"
+ ),
+ HttpResponseStatus.OK,
+ ImmutableMap.of(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON),
Review Comment:
Nit: use `Map.of()` as you are already using it elsewhere in this method.
##########
server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java:
##########
@@ -476,38 +469,20 @@ private List<LookupBean> getLookupListFromCoordinator(String tier)
@Nullable
private Map<String, LookupExtractorFactoryContainer> tryGetLookupListFromCoordinator(String tier)
- throws IOException, InterruptedException
{
- 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()
- );
+ try {
+ return FutureUtils.getUnchecked(
+ coordinatorClient.fetchLookupsForTier(tier), true);
}
-
- // 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
+ catch (Exception e) {
+ HttpResponseException httpEx = (HttpResponseException) e.getCause();
Review Comment:
We don't know if the cause will always be an `HttpResponseException`.
Use `Throwables.getRootCause()` and then check if it is an `HttpResponseException`.
##########
server/src/test/java/org/apache/druid/client/coordinator/CoordinatorClientImplTest.java:
##########
@@ -450,4 +465,35 @@ public void test_getCoordinatorDynamicConfig() throws Exception
coordinatorClient.getCoordinatorDynamicConfig().get()
);
}
+
+ @Test
+ public void test_fetchLookupsForTier_detailedEnabled() 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?detailed=true"
+ ),
+ HttpResponseStatus.OK,
+ ImmutableMap.of(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON),
+ DefaultObjectMapper.INSTANCE.writeValueAsBytes(lookups)
+ );
+
+ Assert.assertEquals(
+ lookups,
+ coordinatorClient.fetchLookupsForTier("country_code").get()
Review Comment:
Nit: `country_code` doesn't sound like a tier name for historicals, use `lookupTier`, `default_tier` or `cold_tier` instead.
##########
server/src/test/java/org/apache/druid/client/coordinator/CoordinatorClientImplTest.java:
##########
@@ -450,4 +465,35 @@ public void test_getCoordinatorDynamicConfig() throws Exception
coordinatorClient.getCoordinatorDynamicConfig().get()
);
}
+
+ @Test
+ public void test_fetchLookupsForTier_detailedEnabled() 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?detailed=true"
+ ),
+ HttpResponseStatus.OK,
+ ImmutableMap.of(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON),
+ DefaultObjectMapper.INSTANCE.writeValueAsBytes(lookups)
+ );
+
+ Assert.assertEquals(
+ lookups,
+ coordinatorClient.fetchLookupsForTier("country_code").get()
Review Comment:
Maybe use `FutureUtils.getUnchecked()` for consistency.
##########
server/src/main/java/org/apache/druid/client/coordinator/CoordinatorClientImpl.java:
##########
@@ -46,10 +50,12 @@
import javax.annotation.Nullable;
import java.util.ArrayList;
import java.util.List;
+import java.util.Map;
import java.util.Set;
public class CoordinatorClientImpl implements CoordinatorClient
{
+ private static final Logger LOG = new Logger(CoordinatorClientImpl.class);
Review Comment:
Not needed anymore.
##########
server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java:
##########
@@ -476,38 +469,20 @@ private List<LookupBean> getLookupListFromCoordinator(String tier)
@Nullable
private Map<String, LookupExtractorFactoryContainer> tryGetLookupListFromCoordinator(String tier)
- throws IOException, InterruptedException
{
- 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()
- );
+ try {
+ return FutureUtils.getUnchecked(
+ coordinatorClient.fetchLookupsForTier(tier), true);
}
-
- // 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
+ catch (Exception e) {
+ HttpResponseException httpEx = (HttpResponseException) e.getCause();
+ LOG.warn(
Review Comment:
If the error is 404, no found, we need to return `null` to retain original code behaviour.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Use CoordinatorClient for fetching tier lookup (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18142:
URL: https://github.com/apache/druid/pull/18142#discussion_r2151410146
##########
server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java:
##########
@@ -475,12 +477,18 @@ private Map<String, LookupExtractorFactoryContainer> tryGetLookupListFromCoordin
coordinatorClient.fetchLookupsForTier(tier), true);
}
catch (Exception e) {
- HttpResponseException httpEx = (HttpResponseException) e.getCause();
- LOG.warn(
- "No lookups found for tier [%s], status [%s]",
- tier,
- httpEx.getResponse().getStatus()
- );
+ Throwable rootCause = Throwables.getRootCause(e);
+ if (rootCause instanceof HttpResponseException) {
+ HttpResponseException httpEx = (HttpResponseException) rootCause;
Review Comment:
Nit:
```suggestion
final HttpResponseException httpException = (HttpResponseException) rootCause;
```
##########
server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java:
##########
@@ -476,38 +471,26 @@ private List<LookupBean> getLookupListFromCoordinator(String tier)
@Nullable
private Map<String, LookupExtractorFactoryContainer> tryGetLookupListFromCoordinator(String tier)
- throws IOException, InterruptedException
{
- 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()
- );
+ try {
+ return FutureUtils.getUnchecked(
+ coordinatorClient.fetchLookupsForTier(tier), true);
Review Comment:
formatting:
```suggestion
return FutureUtils.getUnchecked(coordinatorClient.fetchLookupsForTier(tier), true);
```
##########
server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java:
##########
@@ -476,38 +471,26 @@ private List<LookupBean> getLookupListFromCoordinator(String tier)
@Nullable
private Map<String, LookupExtractorFactoryContainer> tryGetLookupListFromCoordinator(String tier)
- throws IOException, InterruptedException
{
- 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()
- );
+ try {
+ return FutureUtils.getUnchecked(
+ coordinatorClient.fetchLookupsForTier(tier), true);
}
-
- // 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
- );
+ catch (Exception e) {
+ Throwable rootCause = Throwables.getRootCause(e);
+ if (rootCause instanceof HttpResponseException) {
+ HttpResponseException httpEx = (HttpResponseException) rootCause;
+ if (httpEx.getResponse().getStatus().equals(HttpResponseStatus.NOT_FOUND)) {
+ LOG.info(
+ "No lookups found for tier [%s], status [%s]",
+ tier,
+ httpEx.getResponse().getStatus()
+ );
+ return null;
+ }
Review Comment:
The catch clause should re-throw the exception if the cause was not a 404.
##########
server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java:
##########
@@ -476,38 +471,26 @@ private List<LookupBean> getLookupListFromCoordinator(String tier)
@Nullable
private Map<String, LookupExtractorFactoryContainer> tryGetLookupListFromCoordinator(String tier)
- throws IOException, InterruptedException
{
- 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()
- );
+ try {
+ return FutureUtils.getUnchecked(
+ coordinatorClient.fetchLookupsForTier(tier), true);
}
-
- // 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
- );
+ catch (Exception e) {
+ Throwable rootCause = Throwables.getRootCause(e);
+ if (rootCause instanceof HttpResponseException) {
+ HttpResponseException httpEx = (HttpResponseException) rootCause;
+ if (httpEx.getResponse().getStatus().equals(HttpResponseStatus.NOT_FOUND)) {
+ LOG.info(
+ "No lookups found for tier [%s], status [%s]",
+ tier,
+ httpEx.getResponse().getStatus()
+ );
+ return null;
+ }
+ }
}
+ return Map.of();
Review Comment:
We should not return empty in any case.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Use CoordinatorClient for fetching tier lookup (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Use CoordinatorClient for fetching tier lookup (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18142:
URL: https://github.com/apache/druid/pull/18142#discussion_r2151344702
##########
server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java:
##########
@@ -476,38 +469,20 @@ private List<LookupBean> getLookupListFromCoordinator(String tier)
@Nullable
private Map<String, LookupExtractorFactoryContainer> tryGetLookupListFromCoordinator(String tier)
- throws IOException, InterruptedException
{
- 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()
- );
+ try {
+ return FutureUtils.getUnchecked(
+ coordinatorClient.fetchLookupsForTier(tier), true);
}
-
- // 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
+ catch (Exception e) {
+ HttpResponseException httpEx = (HttpResponseException) e.getCause();
+ LOG.warn(
Review Comment:
No, we need to continue doing what the old code was doing.
1. NOT_FOUND: return null
2, OK: return value
3. else: throw exception
2 and 3 will be done automatically by the combination of `CoordinatorClient.get()` and `FutureUtils.getUnchecked`.
We just need to handle 1 separately.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Use CoordinatorClient for fetching tier lookup (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #18142:
URL: https://github.com/apache/druid/pull/18142#discussion_r2150055339
##########
server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java:
##########
@@ -476,38 +469,21 @@
@Nullable
private Map<String, LookupExtractorFactoryContainer> tryGetLookupListFromCoordinator(String tier)
- throws IOException, InterruptedException
{
- 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()
- );
+ try {
+ return FutureUtils.getUnchecked(
+ coordinatorClient.fetchLookupsForTier(tier), true);
}
-
- // 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
+ catch (Exception e) {
+ HttpResponseException httpEx = (HttpResponseException) e.getCause();
+ LOG.warn(
+ "No lookups found for tier [%s], status [%s], response [%s]",
+ tier,
+ httpEx.getResponse().getStatus(),
+ httpEx.getResponse()
Review Comment:
## Use of default toString()
Default toString(): StringFullResponseHolder inherits toString() from Object, and so is not suitable for printing.
[Show more details](https://github.com/apache/druid/security/code-scanning/9238)
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Use CoordinatorClient for fetching tier lookup (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18142:
URL: https://github.com/apache/druid/pull/18142#discussion_r2151206975
##########
server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java:
##########
@@ -476,38 +469,20 @@ private List<LookupBean> getLookupListFromCoordinator(String tier)
@Nullable
private Map<String, LookupExtractorFactoryContainer> tryGetLookupListFromCoordinator(String tier)
- throws IOException, InterruptedException
{
- 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()
- );
+ try {
+ return FutureUtils.getUnchecked(
+ coordinatorClient.fetchLookupsForTier(tier), true);
}
-
- // 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
+ catch (Exception e) {
+ HttpResponseException httpEx = (HttpResponseException) e.getCause();
+ LOG.warn(
Review Comment:
If the error is 404 "not found", we need to return `null` to retain original code behaviour.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Use CoordinatorClient for fetching tier lookup (druid)
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on code in PR #18142:
URL: https://github.com/apache/druid/pull/18142#discussion_r2151322795
##########
server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java:
##########
@@ -476,38 +469,20 @@ private List<LookupBean> getLookupListFromCoordinator(String tier)
@Nullable
private Map<String, LookupExtractorFactoryContainer> tryGetLookupListFromCoordinator(String tier)
- throws IOException, InterruptedException
{
- 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()
- );
+ try {
+ return FutureUtils.getUnchecked(
+ coordinatorClient.fetchLookupsForTier(tier), true);
}
-
- // 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
+ catch (Exception e) {
+ HttpResponseException httpEx = (HttpResponseException) e.getCause();
+ LOG.warn(
Review Comment:
We need to be sending a null map regardless of HTTP status in the event it's 404 / non 200. So just retuning a `null` would be cleaner imo. wdyt?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org