From 6350a9154b78cb5018c47b838405025fd1dd4d21 Mon Sep 17 00:00:00 2001 From: Atul Mohan Date: Mon, 2 Jun 2025 10:40:46 -0700 Subject: [PATCH 1/3] Validate lookup loading spec during lookup addition --- .../query/lookup/LookupReferencesManager.java | 7 +++++++ .../lookup/LookupReferencesManagerTest.java | 21 +++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java b/server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java index 80dd522b0104..24bf8186032f 100644 --- a/server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java +++ b/server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java @@ -282,6 +282,13 @@ public void stop() public void add(String lookupName, LookupExtractorFactoryContainer lookupExtractorFactoryContainer) { Preconditions.checkState(lifecycleLock.awaitStarted(1, TimeUnit.MILLISECONDS)); + LookupLoadingSpec lookupLoadingSpec = lookupListeningAnnouncerConfig.getLookupLoadingSpec(); + if (lookupLoadingSpec.getMode() == LookupLoadingSpec.Mode.NONE || + (lookupLoadingSpec.getMode() == LookupLoadingSpec.Mode.ONLY_REQUIRED + && !lookupLoadingSpec.getLookupsToLoad().contains(lookupName))) { + LOG.warn("Skipping notice to add lookup [%s] : current lookup loading mode does not allow it.", lookupName); + return; + } addNotice(new LoadNotice(lookupName, lookupExtractorFactoryContainer, lookupConfig.getLookupStartRetries())); } diff --git a/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java b/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java index 992c82022091..cb8fcb585a5b 100644 --- a/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java +++ b/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java @@ -849,6 +849,27 @@ public void testCoordinatorLoadSubsetOfLookups() throws Exception Assert.assertFalse(lookupReferencesManager.get("testLookup3").isPresent()); } + @Test + public void testAddWithLoadingSpec() throws Exception + { + LookupLoadingSpec loadingSpec = LookupLoadingSpec.loadOnly(ImmutableSet.of("testLookup1")); + Map lookupMap = + getLookupMapForSelectiveLoadingOfLookups(loadingSpec); + + LookupExtractorFactoryContainer container2 = new LookupExtractorFactoryContainer( + "0", + new MapLookupExtractorFactory(ImmutableMap.of("key2", "value2"), true + ) + ); + EasyMock.reset(config); + EasyMock.expect(config.getLookupLoadingSpec()).andReturn(loadingSpec); + EasyMock.replay(config); + lookupReferencesManager.add("testLookup2", container2); + lookupReferencesManager.handlePendingNotices(); + + Assert.assertEquals(ImmutableSet.of("testLookup1"), lookupReferencesManager.getAllLookupNames()); + } + @Test public void testLoadLookupOnCoordinatorFailure() throws Exception { From 12536b0e4109dcd80e5e34404255cbaccb4cb279 Mon Sep 17 00:00:00 2001 From: Atul Mohan Date: Mon, 2 Jun 2025 11:20:48 -0700 Subject: [PATCH 2/3] Fix assign --- .../apache/druid/query/lookup/LookupReferencesManagerTest.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java b/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java index cb8fcb585a5b..367da38aa0fd 100644 --- a/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java +++ b/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java @@ -853,8 +853,7 @@ public void testCoordinatorLoadSubsetOfLookups() throws Exception public void testAddWithLoadingSpec() throws Exception { LookupLoadingSpec loadingSpec = LookupLoadingSpec.loadOnly(ImmutableSet.of("testLookup1")); - Map lookupMap = - getLookupMapForSelectiveLoadingOfLookups(loadingSpec); + getLookupMapForSelectiveLoadingOfLookups(loadingSpec); LookupExtractorFactoryContainer container2 = new LookupExtractorFactoryContainer( "0", From 6060615ab6e50cd8d3761293f29f0ec0d040de31 Mon Sep 17 00:00:00 2001 From: Atul Mohan Date: Mon, 9 Jun 2025 15:03:55 -0700 Subject: [PATCH 3/3] Address comments --- .../query/lookup/LookupReferencesManager.java | 4 +-- .../lookup/LookupReferencesManagerTest.java | 26 ++++++++++++++++--- 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java b/server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java index 24bf8186032f..ae8a2f41acd8 100644 --- a/server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java +++ b/server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java @@ -282,11 +282,11 @@ public void stop() public void add(String lookupName, LookupExtractorFactoryContainer lookupExtractorFactoryContainer) { Preconditions.checkState(lifecycleLock.awaitStarted(1, TimeUnit.MILLISECONDS)); - LookupLoadingSpec lookupLoadingSpec = lookupListeningAnnouncerConfig.getLookupLoadingSpec(); + final LookupLoadingSpec lookupLoadingSpec = lookupListeningAnnouncerConfig.getLookupLoadingSpec(); if (lookupLoadingSpec.getMode() == LookupLoadingSpec.Mode.NONE || (lookupLoadingSpec.getMode() == LookupLoadingSpec.Mode.ONLY_REQUIRED && !lookupLoadingSpec.getLookupsToLoad().contains(lookupName))) { - LOG.warn("Skipping notice to add lookup [%s] : current lookup loading mode does not allow it.", lookupName); + LOG.info("Skipping notice to add lookup [%s] since current lookup loading mode [%s] does not allow it.", lookupName, lookupLoadingSpec.getMode()); return; } addNotice(new LoadNotice(lookupName, lookupExtractorFactoryContainer, lookupConfig.getLookupStartRetries())); diff --git a/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java b/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java index 367da38aa0fd..d5b179176c67 100644 --- a/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java +++ b/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java @@ -46,6 +46,7 @@ import java.util.HashMap; import java.util.Map; import java.util.Optional; +import java.util.Set; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; @@ -850,14 +851,14 @@ public void testCoordinatorLoadSubsetOfLookups() throws Exception } @Test - public void testAddWithLoadingSpec() throws Exception + public void testAddWithRequiredLoadingSpec() throws Exception { LookupLoadingSpec loadingSpec = LookupLoadingSpec.loadOnly(ImmutableSet.of("testLookup1")); getLookupMapForSelectiveLoadingOfLookups(loadingSpec); LookupExtractorFactoryContainer container2 = new LookupExtractorFactoryContainer( "0", - new MapLookupExtractorFactory(ImmutableMap.of("key2", "value2"), true + new MapLookupExtractorFactory(Map.of("key2", "value2"), true ) ); EasyMock.reset(config); @@ -866,7 +867,26 @@ public void testAddWithLoadingSpec() throws Exception lookupReferencesManager.add("testLookup2", container2); lookupReferencesManager.handlePendingNotices(); - Assert.assertEquals(ImmutableSet.of("testLookup1"), lookupReferencesManager.getAllLookupNames()); + Assert.assertEquals(Set.of("testLookup1"), lookupReferencesManager.getAllLookupNames()); + } + + @Test + public void testAddWithNoneLoadingSpec() throws Exception + { + getLookupMapForSelectiveLoadingOfLookups(LookupLoadingSpec.NONE); + + LookupExtractorFactoryContainer container = new LookupExtractorFactoryContainer( + "0", + new MapLookupExtractorFactory(Map.of("key2", "value2"), true + ) + ); + EasyMock.reset(config); + EasyMock.expect(config.getLookupLoadingSpec()).andReturn(LookupLoadingSpec.NONE); + EasyMock.replay(config); + lookupReferencesManager.add("testLookup", container); + lookupReferencesManager.handlePendingNotices(); + + Assert.assertTrue(lookupReferencesManager.getAllLookupNames().isEmpty()); } @Test