From f33b679c05f5358bd8c103a0bb79b27da34ff91c Mon Sep 17 00:00:00 2001 From: Kashif Faraz Date: Sat, 3 May 2025 21:26:52 +0530 Subject: [PATCH 01/18] WIP: Temp changes --- .../common/actions/TaskActionTestKit.java | 1 + .../common/task/IngestionTestBase.java | 3 +- .../SqlSegmentsMetadataManagerProvider.java | 4 +- .../segment/SqlSegmentsMetadataManagerV2.java | 8 +- .../cache/HeapMemorySegmentMetadataCache.java | 83 ++++++++++++++++++- ...exerSQLMetadataStorageCoordinatorTest.java | 1 + ...qlSegmentsMetadataManagerProviderTest.java | 2 +- .../SqlSegmentsMetadataManagerV2Test.java | 5 +- .../HeapMemorySegmentMetadataCacheTest.java | 2 + 9 files changed, 97 insertions(+), 12 deletions(-) diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/common/actions/TaskActionTestKit.java b/indexing-service/src/test/java/org/apache/druid/indexing/common/actions/TaskActionTestKit.java index f5a28d5d67c5..029c50ddc125 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/common/actions/TaskActionTestKit.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/common/actions/TaskActionTestKit.java @@ -176,6 +176,7 @@ private SqlSegmentMetadataTransactionFactory setupTransactionFactory(ObjectMappe objectMapper, Suppliers.ofInstance(new SegmentsMetadataManagerConfig(Period.seconds(1), cacheMode)), Suppliers.ofInstance(metadataStorageTablesConfig), + Suppliers.ofInstance(CentralizedDatasourceSchemaConfig.create()), testDerbyConnector, (poolSize, name) -> new WrappingScheduledExecutorService(name, metadataCachePollExec, false), NoopServiceEmitter.instance() diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IngestionTestBase.java b/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IngestionTestBase.java index 69564ad65063..bc45d206ccfa 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IngestionTestBase.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IngestionTestBase.java @@ -183,7 +183,7 @@ public void setUpIngestionTestBase() throws IOException derbyConnectorRule.getConnector(), () -> new SegmentsMetadataManagerConfig(null, null), derbyConnectorRule.metadataTablesConfigSupplier(), - CentralizedDatasourceSchemaConfig.create(), + CentralizedDatasourceSchemaConfig::create, NoopServiceEmitter.instance(), objectMapper ); @@ -325,6 +325,7 @@ private SqlSegmentMetadataTransactionFactory createTransactionFactory() objectMapper, Suppliers.ofInstance(new SegmentsMetadataManagerConfig(Period.millis(10), cacheMode)), derbyConnectorRule.metadataTablesConfigSupplier(), + Suppliers.ofInstance(CentralizedDatasourceSchemaConfig.create()), derbyConnectorRule.getConnector(), ScheduledExecutors::fixed, NoopServiceEmitter.instance() diff --git a/server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataManagerProvider.java b/server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataManagerProvider.java index f69dc9508086..8491c5242267 100644 --- a/server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataManagerProvider.java +++ b/server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataManagerProvider.java @@ -39,7 +39,7 @@ public class SqlSegmentsMetadataManagerProvider implements SegmentsMetadataManag private final ServiceEmitter serviceEmitter; private final SegmentSchemaCache segmentSchemaCache; private final SegmentMetadataCache segmentMetadataCache; - private final CentralizedDatasourceSchemaConfig centralizedDatasourceSchemaConfig; + private final Supplier centralizedDatasourceSchemaConfig; @Inject public SqlSegmentsMetadataManagerProvider( @@ -50,7 +50,7 @@ public SqlSegmentsMetadataManagerProvider( SQLMetadataConnector connector, Lifecycle lifecycle, SegmentSchemaCache segmentSchemaCache, - CentralizedDatasourceSchemaConfig centralizedDatasourceSchemaConfig, + Supplier centralizedDatasourceSchemaConfig, ServiceEmitter serviceEmitter ) { diff --git a/server/src/main/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2.java b/server/src/main/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2.java index 893950c83622..1298571e0557 100644 --- a/server/src/main/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2.java +++ b/server/src/main/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2.java @@ -63,7 +63,6 @@ public class SqlSegmentsMetadataManagerV2 implements SegmentsMetadataManager private final SegmentsMetadataManager delegate; private final SegmentMetadataCache segmentMetadataCache; private final SegmentsMetadataManagerConfig managerConfig; - private final CentralizedDatasourceSchemaConfig schemaConfig; public SqlSegmentsMetadataManagerV2( SegmentMetadataCache segmentMetadataCache, @@ -71,7 +70,7 @@ public SqlSegmentsMetadataManagerV2( SQLMetadataConnector connector, Supplier managerConfig, Supplier tablesConfig, - CentralizedDatasourceSchemaConfig centralizedDatasourceSchemaConfig, + Supplier centralizedDatasourceSchemaConfig, ServiceEmitter serviceEmitter, ObjectMapper jsonMapper ) @@ -79,14 +78,13 @@ public SqlSegmentsMetadataManagerV2( this.delegate = new SqlSegmentsMetadataManager( jsonMapper, managerConfig, tablesConfig, connector, segmentSchemaCache, - centralizedDatasourceSchemaConfig, serviceEmitter + centralizedDatasourceSchemaConfig.get(), serviceEmitter ); this.managerConfig = managerConfig.get(); this.segmentMetadataCache = segmentMetadataCache; - this.schemaConfig = centralizedDatasourceSchemaConfig; // Segment metadata cache currently cannot handle schema updates - if (segmentMetadataCache.isEnabled() && schemaConfig.isEnabled()) { + if (segmentMetadataCache.isEnabled() && centralizedDatasourceSchemaConfig.isEnabled()) { throw new IllegalArgumentException( "Segment metadata incremental cache['druid.manager.segments.useIncrementalCache']" + " and segment schema cache['druid.centralizedDatasourceSchema.enabled']" diff --git a/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java b/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java index e954a4f8b4aa..5c86f6bca2af 100644 --- a/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java +++ b/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java @@ -48,6 +48,8 @@ import org.apache.druid.metadata.SegmentsMetadataManagerConfig; import org.apache.druid.metadata.SqlSegmentsMetadataQuery; import org.apache.druid.query.DruidMetrics; +import org.apache.druid.segment.SchemaPayload; +import org.apache.druid.segment.metadata.CentralizedDatasourceSchemaConfig; import org.apache.druid.server.http.DataSegmentPlus; import org.apache.druid.timeline.DataSegment; import org.apache.druid.timeline.SegmentId; @@ -70,6 +72,7 @@ import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; +import java.util.stream.Collectors; /** * In-memory implementation of {@link SegmentMetadataCache}. @@ -109,6 +112,7 @@ private enum CacheState private final Duration pollDuration; private final UsageMode cacheMode; private final MetadataStorageTablesConfig tablesConfig; + private final CentralizedDatasourceSchemaConfig schemaConfig; private final SQLMetadataConnector connector; private final ListeningScheduledExecutorService pollExecutor; @@ -141,6 +145,7 @@ public HeapMemorySegmentMetadataCache( ObjectMapper jsonMapper, Supplier config, Supplier tablesConfig, + Supplier schemaConfig, SQLMetadataConnector connector, ScheduledExecutorFactory executorFactory, ServiceEmitter emitter @@ -150,6 +155,7 @@ public HeapMemorySegmentMetadataCache( this.cacheMode = config.get().getCacheUsageMode(); this.pollDuration = config.get().getPollDuration().toStandardDuration(); this.tablesConfig = tablesConfig.get(); + this.schemaConfig = schemaConfig.get(); this.connector = connector; this.pollExecutor = isEnabled() ? MoreExecutors.listeningDecorator(executorFactory.create(1, "SegmentMetadataCache-%s")) @@ -539,6 +545,7 @@ private long syncWithMetadataStore() updateUsedSegmentPayloadsInCache(datasourceToSummary); retrieveAllPendingSegments(datasourceToSummary); updatePendingSegmentsInCache(datasourceToSummary, syncStartTime); + retrieveAllSegmentSchemas(); markCacheSynced(syncStartTime); syncFinishTime.set(DateTimes.nowUtc()); @@ -679,7 +686,8 @@ private void retrieveRequiredUsedSegments( /** * Updates the cache of each datasource with the segment IDs fetched from the * metadata store in {@link #retrieveUsedSegmentIds}. The update done on each - * datasource cache is atomic. + * datasource cache is atomic. Also identifies the segment IDs which have been + * updated in the metadata store and need to be refreshed in the cache. */ private void updateSegmentIdsInCache( Map datasourceToSummary, @@ -870,6 +878,64 @@ private void retrieveAllPendingSegments( } } + /** + * Retrieves all segment schemas from the metadata store irrespective of their + * used status or last updated time. + */ + private void retrieveAllSegmentSchemas() + { + if (!schemaConfig.isEnabled()) { + return; + } + + final Stopwatch fetchDuration = Stopwatch.createStarted(); + final String sql = StringUtils.format( + "SELECT fingerprint, payload FROM %s WHERE version = %s", + tablesConfig.getSegmentSchemasTable(), CentralizedDatasourceSchemaConfig.SCHEMA_VERSION + ); + + final List records = inReadOnlyTransaction( + (handle, status) -> + handle.createQuery(sql) + .setFetchSize(connector.getStreamingFetchSize()) + .map((index, r, ctx) -> mapToSchemaRecord(r)) + .list() + ); + + final Map fingerprintToSchemaPayload = + records.stream().filter(Objects::nonNull).collect(Collectors.toMap(r -> r.fingerprint, r -> r.payload)); + + if (fingerprintToSchemaPayload.size() < records.size()) { + emitMetric("skipped_schemas", records.size() - fingerprintToSchemaPayload.size()); + } + emitMetric("schema_poll_duration", fetchDuration.millisElapsed()); + + // TODO: update in schema cache directly + // to update the schema cache, we would also need to get the current metadata map + // since not all the segments would be fetched in every poll, so we would need to add some delta on top of it + } + + /** + * Tries to parse the fields of the result set into a {@link SegmentSchemaRecord}. + * + * @return null if an error occurred while parsing the result + */ + private SegmentSchemaRecord mapToSchemaRecord(ResultSet resultSet) + { + String fingerprint = null; + try { + fingerprint = resultSet.getString("fingerprint"); + return new SegmentSchemaRecord( + fingerprint, + jsonMapper.readValue(resultSet.getBytes("payload"), SchemaPayload.class) + ); + } + catch (Throwable t) { + log.error(t, "Could not read segment schema with fingerprint[%s]", fingerprint); + return null; + } + } + /** * Tries to parse the fields of the result set into a {@link DataSegmentPlus}. * @@ -942,4 +1008,19 @@ private void addPendingSegmentRecord(PendingSegmentRecord record) persistedPendingSegments.add(record); } } + + /** + * Represents a single entry in the segment schemas table. + */ + private static class SegmentSchemaRecord + { + private final String fingerprint; + private final SchemaPayload payload; + + private SegmentSchemaRecord(String fingerprint, SchemaPayload payload) + { + this.fingerprint = fingerprint; + this.payload = payload; + } + } } diff --git a/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java b/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java index f7c5af17ecf4..3953b4137ac3 100644 --- a/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java +++ b/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java @@ -153,6 +153,7 @@ public void setUp() mapper, () -> new SegmentsMetadataManagerConfig(null, cacheMode), derbyConnectorRule.metadataTablesConfigSupplier(), + CentralizedDatasourceSchemaConfig::create, derbyConnector, (corePoolSize, nameFormat) -> new WrappingScheduledExecutorService( nameFormat, diff --git a/server/src/test/java/org/apache/druid/metadata/SqlSegmentsMetadataManagerProviderTest.java b/server/src/test/java/org/apache/druid/metadata/SqlSegmentsMetadataManagerProviderTest.java index 0841f586a5db..6e7122630192 100644 --- a/server/src/test/java/org/apache/druid/metadata/SqlSegmentsMetadataManagerProviderTest.java +++ b/server/src/test/java/org/apache/druid/metadata/SqlSegmentsMetadataManagerProviderTest.java @@ -57,7 +57,7 @@ public void testLifecycleStartCreatesSegmentTables() throws Exception connector, lifecycle, segmentSchemaCache, - CentralizedDatasourceSchemaConfig.create(), + CentralizedDatasourceSchemaConfig::create, NoopServiceEmitter.instance() ); SegmentsMetadataManager manager = provider.get(); diff --git a/server/src/test/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2Test.java b/server/src/test/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2Test.java index 9f339ac59ae1..0f1c510f63e4 100644 --- a/server/src/test/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2Test.java +++ b/server/src/test/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2Test.java @@ -90,6 +90,7 @@ private void initManager( jsonMapper, Suppliers.ofInstance(new SegmentsMetadataManagerConfig(Period.seconds(1), cacheMode)), Suppliers.ofInstance(storageConfig), + Suppliers.ofInstance(CentralizedDatasourceSchemaConfig.create()), connector, (poolSize, name) -> new WrappingScheduledExecutorService(name, segmentMetadataCacheExec, false), emitter @@ -103,7 +104,7 @@ private void initManager( connector, Suppliers.ofInstance(config), derbyConnectorRule.metadataTablesConfigSupplier(), - CentralizedDatasourceSchemaConfig.create(), + CentralizedDatasourceSchemaConfig::create, emitter, jsonMapper ); @@ -184,7 +185,7 @@ public boolean isEnabled() connector, Suppliers.ofInstance(config), derbyConnectorRule.metadataTablesConfigSupplier(), - CentralizedDatasourceSchemaConfig.create(true), + () -> CentralizedDatasourceSchemaConfig.create(true), emitter, jsonMapper ) diff --git a/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java b/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java index 489832840719..cc01b2ac9e9a 100644 --- a/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java +++ b/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java @@ -34,6 +34,7 @@ import org.apache.druid.metadata.TestDerbyConnector; import org.apache.druid.segment.TestDataSource; import org.apache.druid.segment.TestHelper; +import org.apache.druid.segment.metadata.CentralizedDatasourceSchemaConfig; import org.apache.druid.segment.realtime.appenderator.SegmentIdWithShardSpec; import org.apache.druid.server.coordinator.CreateDataSegments; import org.apache.druid.server.coordinator.simulate.BlockingExecutorService; @@ -104,6 +105,7 @@ private void setupTargetWithCaching(SegmentMetadataCache.UsageMode cacheMode) TestHelper.JSON_MAPPER, () -> metadataManagerConfig, derbyConnectorRule.metadataTablesConfigSupplier(), + CentralizedDatasourceSchemaConfig::create, derbyConnector, executorFactory, serviceEmitter From 0ffd2da0ce5dcf0abae8b3eaf1b3c5fd21e51ff7 Mon Sep 17 00:00:00 2001 From: Kashif Faraz Date: Mon, 12 May 2025 20:42:51 +0530 Subject: [PATCH 02/18] Enable both --- .../common/actions/TaskActionTestKit.java | 2 + .../common/task/IngestionTestBase.java | 1 + .../guice/SQLMetadataStorageDruidModule.java | 2 + .../IndexerSQLMetadataStorageCoordinator.java | 14 ++-- .../cache/HeapMemorySegmentMetadataCache.java | 70 ++++++++++++++----- .../segment/metadata/SegmentSchemaCache.java | 2 +- .../metadata/SegmentSchemaManager.java | 26 ++++--- ...exerSQLMetadataStorageCoordinatorTest.java | 2 + ...orageCoordinatorSchemaPersistenceTest.java | 4 +- .../SqlSegmentsMetadataManagerV2Test.java | 2 + .../HeapMemorySegmentMetadataCacheTest.java | 2 + .../KillUnreferencedSegmentSchemaTest.java | 9 ++- .../druid/guice/SegmentSchemaCacheModule.java | 2 - 13 files changed, 100 insertions(+), 38 deletions(-) diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/common/actions/TaskActionTestKit.java b/indexing-service/src/test/java/org/apache/druid/indexing/common/actions/TaskActionTestKit.java index 029c50ddc125..aa6dbcb46eda 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/common/actions/TaskActionTestKit.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/common/actions/TaskActionTestKit.java @@ -40,6 +40,7 @@ import org.apache.druid.metadata.segment.cache.HeapMemorySegmentMetadataCache; import org.apache.druid.metadata.segment.cache.SegmentMetadataCache; import org.apache.druid.segment.metadata.CentralizedDatasourceSchemaConfig; +import org.apache.druid.segment.metadata.SegmentSchemaCache; import org.apache.druid.segment.metadata.SegmentSchemaManager; import org.apache.druid.server.coordinator.simulate.BlockingExecutorService; import org.apache.druid.server.coordinator.simulate.TestDruidLeaderSelector; @@ -177,6 +178,7 @@ private SqlSegmentMetadataTransactionFactory setupTransactionFactory(ObjectMappe Suppliers.ofInstance(new SegmentsMetadataManagerConfig(Period.seconds(1), cacheMode)), Suppliers.ofInstance(metadataStorageTablesConfig), Suppliers.ofInstance(CentralizedDatasourceSchemaConfig.create()), + new SegmentSchemaCache(), testDerbyConnector, (poolSize, name) -> new WrappingScheduledExecutorService(name, metadataCachePollExec, false), NoopServiceEmitter.instance() diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IngestionTestBase.java b/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IngestionTestBase.java index 50c1d90015cc..5f6402ce299b 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IngestionTestBase.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IngestionTestBase.java @@ -326,6 +326,7 @@ private SqlSegmentMetadataTransactionFactory createTransactionFactory() Suppliers.ofInstance(new SegmentsMetadataManagerConfig(Period.millis(10), cacheMode)), derbyConnectorRule.metadataTablesConfigSupplier(), Suppliers.ofInstance(CentralizedDatasourceSchemaConfig.create()), + new SegmentSchemaCache(), derbyConnectorRule.getConnector(), ScheduledExecutors::fixed, NoopServiceEmitter.instance() diff --git a/server/src/main/java/org/apache/druid/guice/SQLMetadataStorageDruidModule.java b/server/src/main/java/org/apache/druid/guice/SQLMetadataStorageDruidModule.java index 5f8197d31c32..a884ce01a96c 100644 --- a/server/src/main/java/org/apache/druid/guice/SQLMetadataStorageDruidModule.java +++ b/server/src/main/java/org/apache/druid/guice/SQLMetadataStorageDruidModule.java @@ -42,6 +42,7 @@ import org.apache.druid.metadata.segment.SqlSegmentMetadataTransactionFactory; import org.apache.druid.metadata.segment.cache.HeapMemorySegmentMetadataCache; import org.apache.druid.metadata.segment.cache.SegmentMetadataCache; +import org.apache.druid.segment.metadata.SegmentSchemaCache; import org.apache.druid.server.audit.AuditManagerConfig; import org.apache.druid.server.audit.AuditSerdeHelper; import org.apache.druid.server.audit.SQLAuditManager; @@ -104,6 +105,7 @@ public void configure(Binder binder) binder.bind(SegmentMetadataCache.class) .to(HeapMemorySegmentMetadataCache.class) .in(LazySingleton.class); + binder.bind(SegmentSchemaCache.class).in(LazySingleton.class); PolyBind.optionBinder(binder, Key.get(SegmentMetadataTransactionFactory.class)) .addBinding(type) diff --git a/server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java b/server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java index e4f84dab233f..32ba9103fbd9 100644 --- a/server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java +++ b/server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java @@ -1631,7 +1631,8 @@ private boolean shouldPersistSchema(SegmentSchemaMapping segmentSchemaMapping) private void persistSchema( final SegmentMetadataTransaction transaction, final Set segments, - final SegmentSchemaMapping segmentSchemaMapping + final SegmentSchemaMapping segmentSchemaMapping, + final DateTime updateTime ) throws JsonProcessingException { if (segmentSchemaMapping.getSchemaVersion() != CentralizedDatasourceSchemaConfig.SCHEMA_VERSION) { @@ -1650,7 +1651,8 @@ private void persistSchema( transaction.getHandle(), dataSource, segmentSchemaMapping.getSchemaVersion(), - segmentSchemaMapping.getSchemaFingerprintToPayloadMap() + segmentSchemaMapping.getSchemaFingerprintToPayloadMap(), + updateTime ); } @@ -1662,10 +1664,11 @@ protected Set insertSegments( { final Set toInsertSegments = new HashSet<>(); try { + final DateTime createdTime = DateTimes.nowUtc(); boolean shouldPersistSchema = shouldPersistSchema(segmentSchemaMapping); if (shouldPersistSchema) { - persistSchema(transaction, segments, segmentSchemaMapping); + persistSchema(transaction, segments, segmentSchemaMapping, createdTime); } final Set segmentIds = segments.stream().map(DataSegment::getId).collect(Collectors.toSet()); @@ -1678,7 +1681,6 @@ protected Set insertSegments( } } - final DateTime createdTime = DateTimes.nowUtc(); final Set usedSegments = findNonOvershadowedSegments(segments); final Set segmentPlusToInsert = toInsertSegments.stream().map(segment -> { @@ -1881,8 +1883,9 @@ private Set insertSegments( Map upgradedFromSegmentIdMap ) throws Exception { + final DateTime createdTime = DateTimes.nowUtc(); if (shouldPersistSchema(segmentSchemaMapping)) { - persistSchema(transaction, segments, segmentSchemaMapping); + persistSchema(transaction, segments, segmentSchemaMapping, createdTime); } // Do not insert segment IDs which already exist @@ -1892,7 +1895,6 @@ private Set insertSegments( s -> !existingSegmentIds.contains(s.getId().toString()) ).collect(Collectors.toSet()); - final DateTime createdTime = DateTimes.nowUtc(); final Set segmentPlusToInsert = segmentsToInsert.stream().map(segment -> { SegmentMetadata segmentMetadata = getSegmentMetadataFromSchemaMappingOrUpgradeMetadata( segment.getId(), diff --git a/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java b/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java index 5c86f6bca2af..746ce4363e91 100644 --- a/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java +++ b/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java @@ -49,7 +49,9 @@ import org.apache.druid.metadata.SqlSegmentsMetadataQuery; import org.apache.druid.query.DruidMetrics; import org.apache.druid.segment.SchemaPayload; +import org.apache.druid.segment.SegmentMetadata; import org.apache.druid.segment.metadata.CentralizedDatasourceSchemaConfig; +import org.apache.druid.segment.metadata.SegmentSchemaCache; import org.apache.druid.server.http.DataSegmentPlus; import org.apache.druid.timeline.DataSegment; import org.apache.druid.timeline.SegmentId; @@ -58,6 +60,7 @@ import org.skife.jdbi.v2.ResultIterator; import org.skife.jdbi.v2.TransactionCallback; +import javax.annotation.Nullable; import java.sql.ResultSet; import java.util.ArrayList; import java.util.HashMap; @@ -112,9 +115,11 @@ private enum CacheState private final Duration pollDuration; private final UsageMode cacheMode; private final MetadataStorageTablesConfig tablesConfig; - private final CentralizedDatasourceSchemaConfig schemaConfig; private final SQLMetadataConnector connector; + private final boolean useSchemaCache; + private final SegmentSchemaCache segmentSchemaCache; + private final ListeningScheduledExecutorService pollExecutor; private final ServiceEmitter emitter; @@ -146,6 +151,7 @@ public HeapMemorySegmentMetadataCache( Supplier config, Supplier tablesConfig, Supplier schemaConfig, + SegmentSchemaCache segmentSchemaCache, SQLMetadataConnector connector, ScheduledExecutorFactory executorFactory, ServiceEmitter emitter @@ -155,7 +161,8 @@ public HeapMemorySegmentMetadataCache( this.cacheMode = config.get().getCacheUsageMode(); this.pollDuration = config.get().getPollDuration().toStandardDuration(); this.tablesConfig = tablesConfig.get(); - this.schemaConfig = schemaConfig.get(); + this.useSchemaCache = schemaConfig.get().isEnabled(); + this.segmentSchemaCache = segmentSchemaCache; this.connector = connector; this.pollExecutor = isEnabled() ? MoreExecutors.listeningDecorator(executorFactory.create(1, "SegmentMetadataCache-%s")) @@ -545,7 +552,7 @@ private long syncWithMetadataStore() updateUsedSegmentPayloadsInCache(datasourceToSummary); retrieveAllPendingSegments(datasourceToSummary); updatePendingSegmentsInCache(datasourceToSummary, syncStartTime); - retrieveAllSegmentSchemas(); + retrieveAllSegmentSchemas(datasourceToSummary); markCacheSynced(syncStartTime); syncFinishTime.set(DateTimes.nowUtc()); @@ -675,7 +682,7 @@ private void retrieveRequiredUsedSegments( CloseableIterator iterator = SqlSegmentsMetadataQuery .forHandle(handle, connector, tablesConfig, jsonMapper) - .retrieveSegmentsByIdIterator(dataSource, segmentIdsToRefresh, false) + .retrieveSegmentsByIdIterator(dataSource, segmentIdsToRefresh, useSchemaCache) ) { iterator.forEachRemaining(summary.usedSegments::add); return 0; @@ -742,10 +749,20 @@ private void retrieveAllUsedSegments( ) { final Stopwatch retrieveDuration = Stopwatch.createStarted(); - final String sql = StringUtils.format( - "SELECT id, payload, created_date, used_status_last_updated FROM %s WHERE used = true", - tablesConfig.getSegmentsTable() - ); + final String sql; + if (useSchemaCache) { + sql = StringUtils.format( + "SELECT id, payload, created_date, used_status_last_updated, schema_fingerprint, num_rows" + + " FROM %s WHERE used = true", + tablesConfig.getSegmentsTable() + ); + } else { + sql = StringUtils.format( + "SELECT id, payload, created_date, used_status_last_updated" + + " FROM %s WHERE used = true", + tablesConfig.getSegmentsTable() + ); + } final int numSkippedSegments = inReadOnlyTransaction((handle, status) -> { try ( @@ -882,9 +899,11 @@ private void retrieveAllPendingSegments( * Retrieves all segment schemas from the metadata store irrespective of their * used status or last updated time. */ - private void retrieveAllSegmentSchemas() + private void retrieveAllSegmentSchemas( + Map datasourceToSummary + ) { - if (!schemaConfig.isEnabled()) { + if (!useSchemaCache) { return; } @@ -908,11 +927,28 @@ private void retrieveAllSegmentSchemas() if (fingerprintToSchemaPayload.size() < records.size()) { emitMetric("skipped_schemas", records.size() - fingerprintToSchemaPayload.size()); } - emitMetric("schema_poll_duration", fetchDuration.millisElapsed()); - // TODO: update in schema cache directly - // to update the schema cache, we would also need to get the current metadata map - // since not all the segments would be fetched in every poll, so we would need to add some delta on top of it + final Map segmentIdToMetadata = new HashMap<>( + segmentSchemaCache.getSegmentMetadataMap() + ); + + // Update the map with the segments fetched in the latest sync + datasourceToSummary.values().forEach( + summary -> summary.usedSegments.forEach(segment -> { + if (segment.getNumRows() != null && segment.getSchemaFingerprint() != null) { + segmentIdToMetadata.put( + segment.getDataSegment().getId(), + new SegmentMetadata(segment.getNumRows(), segment.getSchemaFingerprint()) + ); + } + }) + ); + + segmentSchemaCache.resetSchemaForPublishedSegments( + segmentIdToMetadata, + fingerprintToSchemaPayload + ); + emitMetric("schema_poll_duration", fetchDuration.millisElapsed()); } /** @@ -920,6 +956,7 @@ private void retrieveAllSegmentSchemas() * * @return null if an error occurred while parsing the result */ + @Nullable private SegmentSchemaRecord mapToSchemaRecord(ResultSet resultSet) { String fingerprint = null; @@ -941,6 +978,7 @@ private SegmentSchemaRecord mapToSchemaRecord(ResultSet resultSet) * * @return null if an error occurred while parsing the result */ + @Nullable private DataSegmentPlus mapToSegmentPlus(ResultSet resultSet) { String segmentId = null; @@ -951,8 +989,8 @@ private DataSegmentPlus mapToSegmentPlus(ResultSet resultSet) DateTimes.of(resultSet.getString(3)), SqlSegmentsMetadataQuery.nullAndEmptySafeDate(resultSet.getString(4)), true, - null, - null, + useSchemaCache ? resultSet.getString(5) : null, + useSchemaCache ? (Long) resultSet.getObject(6) : null, null ); } diff --git a/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaCache.java b/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaCache.java index 9a6830225dcd..766a9275cedc 100644 --- a/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaCache.java +++ b/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaCache.java @@ -258,7 +258,7 @@ private boolean isPublishedSegmentSchemaCached(SegmentId segmentId) return false; } - private Map getSegmentMetadataMap() + public Map getSegmentMetadataMap() { return publishedSegmentSchemas.get().segmentIdToMetadata; } diff --git a/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaManager.java b/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaManager.java index de4d85742a48..8c1ed5c59ae3 100644 --- a/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaManager.java +++ b/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaManager.java @@ -34,9 +34,9 @@ import org.apache.druid.segment.SchemaPayload; import org.apache.druid.segment.SchemaPayloadPlus; import org.apache.druid.timeline.SegmentId; +import org.joda.time.DateTime; import org.skife.jdbi.v2.Handle; import org.skife.jdbi.v2.PreparedBatch; -import org.skife.jdbi.v2.TransactionCallback; import java.util.ArrayList; import java.util.Collections; @@ -154,7 +154,8 @@ public void persistSchemaAndUpdateSegmentsTable( final int version ) { - connector.retryTransaction((TransactionCallback) (handle, status) -> { + connector.retryTransaction((handle, status) -> { + final DateTime updateTime = DateTimes.nowUtc(); Map schemaPayloadMap = new HashMap<>(); for (SegmentSchemaMetadataPlus segmentSchema : segmentSchemas) { @@ -163,8 +164,8 @@ public void persistSchemaAndUpdateSegmentsTable( segmentSchema.getSegmentSchemaMetadata().getSchemaPayload() ); } - persistSegmentSchema(handle, dataSource, version, schemaPayloadMap); - updateSegmentWithSchemaInformation(handle, segmentSchemas); + persistSegmentSchema(handle, dataSource, version, schemaPayloadMap, updateTime); + updateSegmentWithSchemaInformation(handle, segmentSchemas, updateTime); return null; }, 1, 3); @@ -177,7 +178,8 @@ public void persistSegmentSchema( final Handle handle, final String dataSource, final int version, - final Map fingerprintSchemaPayloadMap + final Map fingerprintSchemaPayloadMap, + final DateTime updateTime ) throws JsonProcessingException { if (fingerprintSchemaPayloadMap.isEmpty()) { @@ -244,7 +246,7 @@ public void persistSegmentSchema( PreparedBatch schemaInsertBatch = handle.prepareBatch(insertSql); for (List partition : partitionedFingerprints) { for (String fingerprint : partition) { - final String now = DateTimes.nowUtc().toString(); + final String now = updateTime.toString(); schemaInsertBatch.add() .bind("created_date", now) .bind("datasource", dataSource) @@ -280,7 +282,8 @@ public void persistSegmentSchema( */ public void updateSegmentWithSchemaInformation( final Handle handle, - final List batch + final List batch, + final DateTime updateTime ) { log.debug("Updating segment with schemaFingerprint and numRows information: [%s].", batch); @@ -288,7 +291,11 @@ public void updateSegmentWithSchemaInformation( // update schemaFingerprint and numRows in segments table String updateSql = StringUtils.format( - "UPDATE %s SET schema_fingerprint = :schema_fingerprint, num_rows = :num_rows WHERE id = :id", + "UPDATE %s" + + " SET schema_fingerprint = :schema_fingerprint," + + " num_rows = :num_rows," + + " used_status_last_updated = :last_updated" + + " WHERE id = :id", dbTables.getSegmentsTable() ); @@ -304,7 +311,8 @@ public void updateSegmentWithSchemaInformation( segmentUpdateBatch.add() .bind("id", segmentSchema.getSegmentId().toString()) .bind("schema_fingerprint", fingerprint) - .bind("num_rows", segmentSchema.getSegmentSchemaMetadata().getNumRows()); + .bind("num_rows", segmentSchema.getSegmentSchemaMetadata().getNumRows()) + .bind("last_updated", updateTime.toString()); } final int[] affectedRows = segmentUpdateBatch.execute(); diff --git a/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java b/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java index 3953b4137ac3..1543493d860d 100644 --- a/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java +++ b/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java @@ -45,6 +45,7 @@ import org.apache.druid.segment.TestDataSource; import org.apache.druid.segment.metadata.CentralizedDatasourceSchemaConfig; import org.apache.druid.segment.metadata.FingerprintGenerator; +import org.apache.druid.segment.metadata.SegmentSchemaCache; import org.apache.druid.segment.metadata.SegmentSchemaManager; import org.apache.druid.segment.metadata.SegmentSchemaTestUtils; import org.apache.druid.segment.realtime.appenderator.SegmentIdWithShardSpec; @@ -154,6 +155,7 @@ public void setUp() () -> new SegmentsMetadataManagerConfig(null, cacheMode), derbyConnectorRule.metadataTablesConfigSupplier(), CentralizedDatasourceSchemaConfig::create, + new SegmentSchemaCache(), derbyConnector, (corePoolSize, nameFormat) -> new WrappingScheduledExecutorService( nameFormat, diff --git a/server/src/test/java/org/apache/druid/metadata/IndexerSqlMetadataStorageCoordinatorSchemaPersistenceTest.java b/server/src/test/java/org/apache/druid/metadata/IndexerSqlMetadataStorageCoordinatorSchemaPersistenceTest.java index a4eddec3d4ef..6fd536266d04 100644 --- a/server/src/test/java/org/apache/druid/metadata/IndexerSqlMetadataStorageCoordinatorSchemaPersistenceTest.java +++ b/server/src/test/java/org/apache/druid/metadata/IndexerSqlMetadataStorageCoordinatorSchemaPersistenceTest.java @@ -27,6 +27,7 @@ import org.apache.druid.discovery.NodeRole; import org.apache.druid.indexing.overlord.DataSourceMetadata; import org.apache.druid.indexing.overlord.SegmentPublishResult; +import org.apache.druid.java.util.common.DateTimes; import org.apache.druid.java.util.common.Intervals; import org.apache.druid.java.util.common.Pair; import org.apache.druid.metadata.segment.SegmentMetadataTransaction; @@ -449,7 +450,8 @@ public void testAnnounceHistoricalSegments_schemaExists() throws IOException handle, "fooDataSource", CentralizedDatasourceSchemaConfig.SCHEMA_VERSION, - schemaPayloadMapToPerist + schemaPayloadMapToPerist, + DateTimes.nowUtc() ); return null; }); diff --git a/server/src/test/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2Test.java b/server/src/test/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2Test.java index f2bb5a2253fa..d98de1f13619 100644 --- a/server/src/test/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2Test.java +++ b/server/src/test/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2Test.java @@ -35,6 +35,7 @@ import org.apache.druid.metadata.segment.cache.SegmentMetadataCache; import org.apache.druid.segment.TestDataSource; import org.apache.druid.segment.metadata.CentralizedDatasourceSchemaConfig; +import org.apache.druid.segment.metadata.SegmentSchemaCache; import org.apache.druid.server.coordinator.CreateDataSegments; import org.apache.druid.server.coordinator.simulate.BlockingExecutorService; import org.apache.druid.server.coordinator.simulate.WrappingScheduledExecutorService; @@ -91,6 +92,7 @@ private void initManager( Suppliers.ofInstance(new SegmentsMetadataManagerConfig(Period.seconds(1), cacheMode)), Suppliers.ofInstance(storageConfig), Suppliers.ofInstance(CentralizedDatasourceSchemaConfig.create()), + new SegmentSchemaCache(), connector, (poolSize, name) -> new WrappingScheduledExecutorService(name, segmentMetadataCacheExec, false), emitter diff --git a/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java b/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java index cc01b2ac9e9a..f1a5bea26338 100644 --- a/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java +++ b/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java @@ -35,6 +35,7 @@ import org.apache.druid.segment.TestDataSource; import org.apache.druid.segment.TestHelper; import org.apache.druid.segment.metadata.CentralizedDatasourceSchemaConfig; +import org.apache.druid.segment.metadata.SegmentSchemaCache; import org.apache.druid.segment.realtime.appenderator.SegmentIdWithShardSpec; import org.apache.druid.server.coordinator.CreateDataSegments; import org.apache.druid.server.coordinator.simulate.BlockingExecutorService; @@ -106,6 +107,7 @@ private void setupTargetWithCaching(SegmentMetadataCache.UsageMode cacheMode) () -> metadataManagerConfig, derbyConnectorRule.metadataTablesConfigSupplier(), CentralizedDatasourceSchemaConfig::create, + new SegmentSchemaCache(), derbyConnector, executorFactory, serviceEmitter diff --git a/server/src/test/java/org/apache/druid/server/coordinator/duty/KillUnreferencedSegmentSchemaTest.java b/server/src/test/java/org/apache/druid/server/coordinator/duty/KillUnreferencedSegmentSchemaTest.java index c5e2012d02aa..dae4d48a1bfa 100644 --- a/server/src/test/java/org/apache/druid/server/coordinator/duty/KillUnreferencedSegmentSchemaTest.java +++ b/server/src/test/java/org/apache/druid/server/coordinator/duty/KillUnreferencedSegmentSchemaTest.java @@ -229,7 +229,8 @@ public void testKillUnreferencedSchema_repair() handle, "foo", CentralizedDatasourceSchemaConfig.SCHEMA_VERSION, - Collections.singletonMap(fingerprint, schemaPayload) + Collections.singletonMap(fingerprint, schemaPayload), + DateTimes.nowUtc() ); return null; } @@ -304,7 +305,8 @@ public void testKillOlderVersionSchema() handle, "foo", 0, - Collections.singletonMap(fingerprintOldVersion, schemaPayload) + Collections.singletonMap(fingerprintOldVersion, schemaPayload), + DateTimes.nowUtc() ); return null; } @@ -316,7 +318,8 @@ public void testKillOlderVersionSchema() handle, "foo", 1, - Collections.singletonMap(fingerprintNewVersion, schemaPayload) + Collections.singletonMap(fingerprintNewVersion, schemaPayload), + DateTimes.nowUtc() ); return null; } diff --git a/services/src/main/java/org/apache/druid/guice/SegmentSchemaCacheModule.java b/services/src/main/java/org/apache/druid/guice/SegmentSchemaCacheModule.java index 377950b03700..744d56a89e02 100644 --- a/services/src/main/java/org/apache/druid/guice/SegmentSchemaCacheModule.java +++ b/services/src/main/java/org/apache/druid/guice/SegmentSchemaCacheModule.java @@ -44,7 +44,6 @@ import org.apache.druid.segment.metadata.CentralizedDatasourceSchemaConfig; import org.apache.druid.segment.metadata.CoordinatorSegmentMetadataCache; import org.apache.druid.segment.metadata.SegmentMetadataQuerySegmentWalker; -import org.apache.druid.segment.metadata.SegmentSchemaCache; import org.apache.druid.server.QueryScheduler; import org.apache.druid.server.QuerySchedulerProvider; @@ -83,7 +82,6 @@ public void configure(Binder binder) .toProvider(Key.get(QuerySchedulerProvider.class, Global.class)) .in(LazySingleton.class); binder.bind(QuerySchedulerProvider.class).in(LazySingleton.class); - binder.bind(SegmentSchemaCache.class).in(LazySingleton.class); binder.bind(QuerySegmentWalker.class).to(SegmentMetadataQuerySegmentWalker.class).in(LazySingleton.class); LifecycleModule.register(binder, CoordinatorSegmentMetadataCache.class); From a15cbd3b4de359c6465a07d8efbda68f112b7db1 Mon Sep 17 00:00:00 2001 From: Kashif Faraz Date: Mon, 12 May 2025 21:10:22 +0530 Subject: [PATCH 03/18] Fix up compilation and metrics --- .../segment/SqlSegmentsMetadataManagerV2.java | 9 --- .../cache/HeapMemorySegmentMetadataCache.java | 7 ++- .../druid/metadata/segment/cache/Metric.java | 10 ++++ .../segment/metadata/SegmentSchemaCache.java | 4 ++ .../SqlSegmentsMetadataManagerV2Test.java | 60 ++++++++----------- 5 files changed, 45 insertions(+), 45 deletions(-) diff --git a/server/src/main/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2.java b/server/src/main/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2.java index 1298571e0557..9d044962dd79 100644 --- a/server/src/main/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2.java +++ b/server/src/main/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2.java @@ -82,15 +82,6 @@ public SqlSegmentsMetadataManagerV2( ); this.managerConfig = managerConfig.get(); this.segmentMetadataCache = segmentMetadataCache; - - // Segment metadata cache currently cannot handle schema updates - if (segmentMetadataCache.isEnabled() && centralizedDatasourceSchemaConfig.isEnabled()) { - throw new IllegalArgumentException( - "Segment metadata incremental cache['druid.manager.segments.useIncrementalCache']" - + " and segment schema cache['druid.centralizedDatasourceSchema.enabled']" - + " must not be enabled together." - ); - } } /** diff --git a/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java b/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java index 746ce4363e91..5dc79448098b 100644 --- a/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java +++ b/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java @@ -907,6 +907,9 @@ private void retrieveAllSegmentSchemas( return; } + // Emit metrics for the current contents of the cache + segmentSchemaCache.getStats().forEach(this::emitMetric); + final Stopwatch fetchDuration = Stopwatch.createStarted(); final String sql = StringUtils.format( "SELECT fingerprint, payload FROM %s WHERE version = %s", @@ -925,7 +928,7 @@ private void retrieveAllSegmentSchemas( records.stream().filter(Objects::nonNull).collect(Collectors.toMap(r -> r.fingerprint, r -> r.payload)); if (fingerprintToSchemaPayload.size() < records.size()) { - emitMetric("skipped_schemas", records.size() - fingerprintToSchemaPayload.size()); + emitMetric(Metric.SKIPPED_SEGMENT_SCHEMAS, records.size() - fingerprintToSchemaPayload.size()); } final Map segmentIdToMetadata = new HashMap<>( @@ -948,7 +951,7 @@ private void retrieveAllSegmentSchemas( segmentIdToMetadata, fingerprintToSchemaPayload ); - emitMetric("schema_poll_duration", fetchDuration.millisElapsed()); + emitMetric(Metric.RETRIEVE_SEGMENT_SCHEMAS_DURATION_MILLIS, fetchDuration.millisElapsed()); } /** diff --git a/server/src/main/java/org/apache/druid/metadata/segment/cache/Metric.java b/server/src/main/java/org/apache/druid/metadata/segment/cache/Metric.java index 6ce9e245614d..c8e87ca4d55d 100644 --- a/server/src/main/java/org/apache/druid/metadata/segment/cache/Metric.java +++ b/server/src/main/java/org/apache/druid/metadata/segment/cache/Metric.java @@ -105,6 +105,11 @@ private Metric() */ public static final String RETRIEVE_PENDING_SEGMENTS_DURATION_MILLIS = METRIC_NAME_PREFIX + "fetchPending/time"; + /** + * Time taken in milliseconds to fetch all segment schemas from the metadata store. + */ + public static final String RETRIEVE_SEGMENT_SCHEMAS_DURATION_MILLIS = METRIC_NAME_PREFIX + "fetchSchemas/time"; + /** * Time taken to update the datasource snapshot in the cache. */ @@ -148,6 +153,11 @@ private Metric() */ public static final String SKIPPED_SEGMENTS = METRIC_NAME_PREFIX + "skipped"; + /** + * Number of unparseable segment schema records skipped while refreshing the cache. + */ + public static final String SKIPPED_SEGMENT_SCHEMAS = METRIC_NAME_PREFIX + "schema/skipped"; + /** * Number of unparseable pending segment records skipped while refreshing the cache. */ diff --git a/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaCache.java b/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaCache.java index 766a9275cedc..cef72913c498 100644 --- a/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaCache.java +++ b/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaCache.java @@ -258,6 +258,10 @@ private boolean isPublishedSegmentSchemaCached(SegmentId segmentId) return false; } + /** + * @return Immutable map from segment ID to {@link SegmentMetadata} for all + * published used segments currently present in this cache. + */ public Map getSegmentMetadataMap() { return publishedSegmentSchemas.get().segmentIdToMetadata; diff --git a/server/src/test/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2Test.java b/server/src/test/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2Test.java index d98de1f13619..d6dacd5b6354 100644 --- a/server/src/test/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2Test.java +++ b/server/src/test/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2Test.java @@ -21,7 +21,6 @@ import com.google.common.base.Suppliers; import org.apache.druid.client.DataSourcesSnapshot; -import org.apache.druid.error.ExceptionMatcher; import org.apache.druid.java.util.common.DateTimes; import org.apache.druid.java.util.common.granularity.Granularities; import org.apache.druid.java.util.metrics.StubServiceEmitter; @@ -31,7 +30,6 @@ import org.apache.druid.metadata.TestDerbyConnector; import org.apache.druid.metadata.segment.cache.HeapMemorySegmentMetadataCache; import org.apache.druid.metadata.segment.cache.Metric; -import org.apache.druid.metadata.segment.cache.NoopSegmentMetadataCache; import org.apache.druid.metadata.segment.cache.SegmentMetadataCache; import org.apache.druid.segment.TestDataSource; import org.apache.druid.segment.metadata.CentralizedDatasourceSchemaConfig; @@ -41,7 +39,6 @@ import org.apache.druid.server.coordinator.simulate.WrappingScheduledExecutorService; import org.apache.druid.timeline.DataSegment; import org.assertj.core.util.Sets; -import org.hamcrest.MatcherAssert; import org.joda.time.DateTime; import org.joda.time.Period; import org.junit.After; @@ -83,7 +80,8 @@ public void setup() throws Exception } private void initManager( - SegmentMetadataCache.UsageMode cacheMode + SegmentMetadataCache.UsageMode cacheMode, + boolean useSchemaCache ) { segmentMetadataCacheExec = new BlockingExecutorService("test"); @@ -91,7 +89,7 @@ private void initManager( jsonMapper, Suppliers.ofInstance(new SegmentsMetadataManagerConfig(Period.seconds(1), cacheMode)), Suppliers.ofInstance(storageConfig), - Suppliers.ofInstance(CentralizedDatasourceSchemaConfig.create()), + Suppliers.ofInstance(CentralizedDatasourceSchemaConfig.enabled(useSchemaCache)), new SegmentSchemaCache(), connector, (poolSize, name) -> new WrappingScheduledExecutorService(name, segmentMetadataCacheExec, false), @@ -135,7 +133,7 @@ public void tearDown() @Test public void test_manager_usesCachedSegments_ifCacheIsEnabled() { - initManager(SegmentMetadataCache.UsageMode.ALWAYS); + initManager(SegmentMetadataCache.UsageMode.ALWAYS, false); manager.startPollingDatabasePeriodically(); Assert.assertTrue(manager.isPollingDatabasePeriodically()); @@ -149,13 +147,16 @@ public void test_manager_usesCachedSegments_ifCacheIsEnabled() emitter.verifyNotEmitted("segment/poll/time"); emitter.verifyNotEmitted("segment/pollWithSchema/time"); + emitter.verifyNotEmitted(Metric.RETRIEVE_SEGMENT_SCHEMAS_DURATION_MILLIS); + emitter.verifyNotEmitted("segment/schemaCache/used/count"); + emitter.verifyEmitted(Metric.SYNC_DURATION_MILLIS, 2); } @Test public void test_manager_pollsSegments_ifCacheIsDisabled() { - initManager(SegmentMetadataCache.UsageMode.NEVER); + initManager(SegmentMetadataCache.UsageMode.NEVER, false); manager.startPollingDatabasePeriodically(); Assert.assertTrue(manager.isPollingDatabasePeriodically()); @@ -170,34 +171,25 @@ public void test_manager_pollsSegments_ifCacheIsDisabled() } @Test - public void test_manager_throwsException_ifBothCacheAndSchemaAreEnabled() + public void test_manager_pollsSegmentsAndSchemas_ifBothCacheAndSchemaAreEnabled() { - MatcherAssert.assertThat( - Assert.assertThrows( - IllegalArgumentException.class, - () -> new SqlSegmentsMetadataManagerV2( - new NoopSegmentMetadataCache() { - @Override - public boolean isEnabled() - { - return true; - } - }, - segmentSchemaCache, - connector, - Suppliers.ofInstance(config), - derbyConnectorRule.metadataTablesConfigSupplier(), - () -> CentralizedDatasourceSchemaConfig.enabled(true), - emitter, - jsonMapper - ) - ), - ExceptionMatcher.of(IllegalArgumentException.class).expectMessageIs( - "Segment metadata incremental cache['druid.manager.segments.useIncrementalCache']" - + " and segment schema cache['druid.centralizedDatasourceSchema.enabled']" - + " must not be enabled together." - ) - ); + initManager(SegmentMetadataCache.UsageMode.ALWAYS, true); + + manager.startPollingDatabasePeriodically(); + Assert.assertTrue(manager.isPollingDatabasePeriodically()); + + syncSegmentMetadataCache(); + verifyDatasourceSnapshot(); + + // isPolling returns true even after stop since cache is still polling the metadata store + manager.stopPollingDatabasePeriodically(); + Assert.assertTrue(manager.isPollingDatabasePeriodically()); + + emitter.verifyNotEmitted("segment/poll/time"); + emitter.verifyNotEmitted("segment/pollWithSchema/time"); + emitter.verifyEmitted(Metric.SYNC_DURATION_MILLIS, 2); + emitter.verifyEmitted(Metric.RETRIEVE_SEGMENT_SCHEMAS_DURATION_MILLIS, 2); + emitter.verifyEmitted("segment/schemaCache/used/count", 2); } private void verifyDatasourceSnapshot() From 93dd1d1166c7adea4778ea16b0d28aee457d5719 Mon Sep 17 00:00:00 2001 From: Kashif Faraz Date: Mon, 12 May 2025 21:16:39 +0530 Subject: [PATCH 04/18] Enable ITs to use both features together --- ...docker-compose.cds-coordinator-metadata-query-disabled.yml | 4 ++-- .../docker-compose.cds-task-schema-publish-disabled.yml | 4 ++-- .../docker/docker-compose.centralized-datasource-schema.yml | 4 ++-- .../metadata/segment/SqlSegmentsMetadataManagerV2Test.java | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/integration-tests/docker/docker-compose.cds-coordinator-metadata-query-disabled.yml b/integration-tests/docker/docker-compose.cds-coordinator-metadata-query-disabled.yml index 60cff61472d7..b435c290ef84 100644 --- a/integration-tests/docker/docker-compose.cds-coordinator-metadata-query-disabled.yml +++ b/integration-tests/docker/docker-compose.cds-coordinator-metadata-query-disabled.yml @@ -40,7 +40,7 @@ services: - druid_centralizedDatasourceSchema_backFillPeriod=15000 - druid_coordinator_segmentMetadata_metadataRefreshPeriod=PT15S - druid_coordinator_segmentMetadata_disableSegmentMetadataQueries=true - - druid_manager_segments_useIncrementalCache=never + - druid_manager_segments_useIncrementalCache=always depends_on: - druid-overlord - druid-metadata-storage @@ -53,7 +53,7 @@ services: environment: - DRUID_INTEGRATION_TEST_GROUP=${DRUID_INTEGRATION_TEST_GROUP} - druid_centralizedDatasourceSchema_enabled=true - - druid_manager_segments_useIncrementalCache=never + - druid_manager_segments_useIncrementalCache=always depends_on: - druid-metadata-storage - druid-zookeeper-kafka diff --git a/integration-tests/docker/docker-compose.cds-task-schema-publish-disabled.yml b/integration-tests/docker/docker-compose.cds-task-schema-publish-disabled.yml index f7923f368d89..b423ad9f6588 100644 --- a/integration-tests/docker/docker-compose.cds-task-schema-publish-disabled.yml +++ b/integration-tests/docker/docker-compose.cds-task-schema-publish-disabled.yml @@ -40,7 +40,7 @@ services: - druid_centralizedDatasourceSchema_backFillEnabled=true - druid_centralizedDatasourceSchema_backFillPeriod=15000 - druid_coordinator_segmentMetadata_metadataRefreshPeriod=PT15S - - druid_manager_segments_useIncrementalCache=never + - druid_manager_segments_useIncrementalCache=always depends_on: - druid-overlord - druid-metadata-storage @@ -54,7 +54,7 @@ services: - DRUID_INTEGRATION_TEST_GROUP=${DRUID_INTEGRATION_TEST_GROUP} - druid_centralizedDatasourceSchema_enabled=true - druid_centralizedDatasourceSchema_taskSchemaPublishDisabled=true - - druid_manager_segments_useIncrementalCache=never + - druid_manager_segments_useIncrementalCache=always depends_on: - druid-metadata-storage - druid-zookeeper-kafka diff --git a/integration-tests/docker/docker-compose.centralized-datasource-schema.yml b/integration-tests/docker/docker-compose.centralized-datasource-schema.yml index 8674a5620126..b47ce1533af8 100644 --- a/integration-tests/docker/docker-compose.centralized-datasource-schema.yml +++ b/integration-tests/docker/docker-compose.centralized-datasource-schema.yml @@ -39,7 +39,7 @@ services: - druid_centralizedDatasourceSchema_backFillEnabled=true - druid_centralizedDatasourceSchema_backFillPeriod=15000 - druid_coordinator_segmentMetadata_metadataRefreshPeriod=PT15S - - druid_manager_segments_useIncrementalCache=never + - druid_manager_segments_useIncrementalCache=always depends_on: - druid-overlord - druid-metadata-storage @@ -52,7 +52,7 @@ services: environment: - DRUID_INTEGRATION_TEST_GROUP=${DRUID_INTEGRATION_TEST_GROUP} - druid_centralizedDatasourceSchema_enabled=true - - druid_manager_segments_useIncrementalCache=never + - druid_manager_segments_useIncrementalCache=always depends_on: - druid-metadata-storage - druid-zookeeper-kafka diff --git a/server/src/test/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2Test.java b/server/src/test/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2Test.java index d6dacd5b6354..6b02e63498f1 100644 --- a/server/src/test/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2Test.java +++ b/server/src/test/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2Test.java @@ -171,7 +171,7 @@ public void test_manager_pollsSegments_ifCacheIsDisabled() } @Test - public void test_manager_pollsSegmentsAndSchemas_ifBothCacheAndSchemaAreEnabled() + public void test_manager_usesCachedSegmentsAndSchemas_ifBothCacheAndSchemaAreEnabled() { initManager(SegmentMetadataCache.UsageMode.ALWAYS, true); From a9973d8ac28f5e164e4cf5b01cdca9f55f3c2574 Mon Sep 17 00:00:00 2001 From: Kashif Faraz Date: Thu, 15 May 2025 23:47:09 +0530 Subject: [PATCH 05/18] Poll only used and recently updated schemas --- .../common/actions/TaskActionTestKit.java | 5 ++- .../common/task/IngestionTestBase.java | 4 +- .../cache/HeapMemorySegmentMetadataCache.java | 37 ++++++++++++++----- .../segment/metadata/SegmentSchemaCache.java | 6 ++- ...etadataStorageCoordinatorReadOnlyTest.java | 4 ++ ...exerSQLMetadataStorageCoordinatorTest.java | 4 +- .../SqlSegmentsMetadataManagerV2Test.java | 9 +++-- .../HeapMemorySegmentMetadataCacheTest.java | 2 + 8 files changed, 54 insertions(+), 17 deletions(-) diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/common/actions/TaskActionTestKit.java b/indexing-service/src/test/java/org/apache/druid/indexing/common/actions/TaskActionTestKit.java index aa6dbcb46eda..b9522cc0f9ee 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/common/actions/TaskActionTestKit.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/common/actions/TaskActionTestKit.java @@ -173,7 +173,10 @@ private SqlSegmentMetadataTransactionFactory setupTransactionFactory(ObjectMappe = useSegmentMetadataCache ? SegmentMetadataCache.UsageMode.ALWAYS : SegmentMetadataCache.UsageMode.NEVER; + + final Set nodeRoles = Set.of(NodeRole.OVERLORD); segmentMetadataCache = new HeapMemorySegmentMetadataCache( + nodeRoles, objectMapper, Suppliers.ofInstance(new SegmentsMetadataManagerConfig(Period.seconds(1), cacheMode)), Suppliers.ofInstance(metadataStorageTablesConfig), @@ -192,7 +195,7 @@ private SqlSegmentMetadataTransactionFactory setupTransactionFactory(ObjectMappe metadataStorageTablesConfig, testDerbyConnector, leaderSelector, - Set.of(NodeRole.OVERLORD), + nodeRoles, segmentMetadataCache, NoopServiceEmitter.instance() ) diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IngestionTestBase.java b/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IngestionTestBase.java index 5f6402ce299b..04a6f53a2da5 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IngestionTestBase.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IngestionTestBase.java @@ -321,7 +321,9 @@ private SqlSegmentMetadataTransactionFactory createTransactionFactory() = useSegmentMetadataCache ? SegmentMetadataCache.UsageMode.ALWAYS : SegmentMetadataCache.UsageMode.NEVER; + final Set nodeRoles = Set.of(NodeRole.OVERLORD); segmentMetadataCache = new HeapMemorySegmentMetadataCache( + nodeRoles, objectMapper, Suppliers.ofInstance(new SegmentsMetadataManagerConfig(Period.millis(10), cacheMode)), derbyConnectorRule.metadataTablesConfigSupplier(), @@ -340,7 +342,7 @@ private SqlSegmentMetadataTransactionFactory createTransactionFactory() derbyConnectorRule.metadataTablesConfigSupplier().get(), derbyConnectorRule.getConnector(), leaderSelector, - Set.of(NodeRole.OVERLORD), + nodeRoles, segmentMetadataCache, NoopServiceEmitter.instance() ); diff --git a/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java b/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java index 5dc79448098b..fb74a30001ca 100644 --- a/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java +++ b/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java @@ -31,6 +31,7 @@ import com.google.errorprone.annotations.concurrent.GuardedBy; import com.google.inject.Inject; import org.apache.druid.client.DataSourcesSnapshot; +import org.apache.druid.discovery.NodeRole; import org.apache.druid.error.DruidException; import org.apache.druid.error.InternalServerError; import org.apache.druid.java.util.common.DateTimes; @@ -147,6 +148,7 @@ private enum CacheState @Inject public HeapMemorySegmentMetadataCache( + Set nodeRoles, ObjectMapper jsonMapper, Supplier config, Supplier tablesConfig, @@ -161,7 +163,7 @@ public HeapMemorySegmentMetadataCache( this.cacheMode = config.get().getCacheUsageMode(); this.pollDuration = config.get().getPollDuration().toStandardDuration(); this.tablesConfig = tablesConfig.get(); - this.useSchemaCache = schemaConfig.get().isEnabled(); + this.useSchemaCache = schemaConfig.get().isEnabled() && nodeRoles.contains(NodeRole.COORDINATOR); this.segmentSchemaCache = segmentSchemaCache; this.connector = connector; this.pollExecutor = isEnabled() @@ -552,7 +554,13 @@ private long syncWithMetadataStore() updateUsedSegmentPayloadsInCache(datasourceToSummary); retrieveAllPendingSegments(datasourceToSummary); updatePendingSegmentsInCache(datasourceToSummary, syncStartTime); - retrieveAllSegmentSchemas(datasourceToSummary); + + if (syncFinishTime.get() == null) { + retrieveUsedSegmentSchemasUpdatedAfter(DateTimes.COMPARE_DATE_AS_STRING_MIN, datasourceToSummary); + } else { + retrieveUsedSegmentSchemasUpdatedAfter(syncStartTime, datasourceToSummary); + } + markCacheSynced(syncStartTime); syncFinishTime.set(DateTimes.nowUtc()); @@ -899,7 +907,8 @@ private void retrieveAllPendingSegments( * Retrieves all segment schemas from the metadata store irrespective of their * used status or last updated time. */ - private void retrieveAllSegmentSchemas( + private void retrieveUsedSegmentSchemasUpdatedAfter( + DateTime minUpdateTime, Map datasourceToSummary ) { @@ -910,32 +919,40 @@ private void retrieveAllSegmentSchemas( // Emit metrics for the current contents of the cache segmentSchemaCache.getStats().forEach(this::emitMetric); - final Stopwatch fetchDuration = Stopwatch.createStarted(); + final Stopwatch schemaSyncDuration = Stopwatch.createStarted(); final String sql = StringUtils.format( - "SELECT fingerprint, payload FROM %s WHERE version = %s", + "SELECT fingerprint, payload FROM %s" + + " WHERE version = %s AND used = true" + + " AND used_status_last_updated >= :minUpdateTime", tablesConfig.getSegmentSchemasTable(), CentralizedDatasourceSchemaConfig.SCHEMA_VERSION ); final List records = inReadOnlyTransaction( (handle, status) -> handle.createQuery(sql) + .bind("minUpdateTime", minUpdateTime.toString()) .setFetchSize(connector.getStreamingFetchSize()) .map((index, r, ctx) -> mapToSchemaRecord(r)) .list() ); - final Map fingerprintToSchemaPayload = + final Map updatedFingerprintToSchemaPayload = records.stream().filter(Objects::nonNull).collect(Collectors.toMap(r -> r.fingerprint, r -> r.payload)); - if (fingerprintToSchemaPayload.size() < records.size()) { - emitMetric(Metric.SKIPPED_SEGMENT_SCHEMAS, records.size() - fingerprintToSchemaPayload.size()); + if (updatedFingerprintToSchemaPayload.size() < records.size()) { + emitMetric(Metric.SKIPPED_SEGMENT_SCHEMAS, records.size() - updatedFingerprintToSchemaPayload.size()); } + // Build a map for the currently cached entries final Map segmentIdToMetadata = new HashMap<>( segmentSchemaCache.getSegmentMetadataMap() ); + final Map fingerprintToSchemaPayload = new HashMap<>( + segmentSchemaCache.getSchemaPayloadMap() + ); - // Update the map with the segments fetched in the latest sync + // Update the map with the segments fetched in this sync + fingerprintToSchemaPayload.putAll(updatedFingerprintToSchemaPayload); datasourceToSummary.values().forEach( summary -> summary.usedSegments.forEach(segment -> { if (segment.getNumRows() != null && segment.getSchemaFingerprint() != null) { @@ -951,7 +968,7 @@ private void retrieveAllSegmentSchemas( segmentIdToMetadata, fingerprintToSchemaPayload ); - emitMetric(Metric.RETRIEVE_SEGMENT_SCHEMAS_DURATION_MILLIS, fetchDuration.millisElapsed()); + emitMetric(Metric.RETRIEVE_SEGMENT_SCHEMAS_DURATION_MILLIS, schemaSyncDuration.millisElapsed()); } /** diff --git a/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaCache.java b/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaCache.java index cef72913c498..4e86593eb7be 100644 --- a/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaCache.java +++ b/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaCache.java @@ -267,7 +267,11 @@ public Map getSegmentMetadataMap() return publishedSegmentSchemas.get().segmentIdToMetadata; } - private Map getSchemaPayloadMap() + /** + * @return Immutable map from schema fingerprint to {@link SchemaPayload} for + * all schema fingerprints currently present in this cache. + */ + public Map getSchemaPayloadMap() { return publishedSegmentSchemas.get().schemaFingerprintToPayload; } diff --git a/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorReadOnlyTest.java b/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorReadOnlyTest.java index 5f5d0db9f582..8ab0d3d5ee7e 100644 --- a/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorReadOnlyTest.java +++ b/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorReadOnlyTest.java @@ -35,6 +35,7 @@ import org.apache.druid.segment.TestDataSource; import org.apache.druid.segment.TestHelper; import org.apache.druid.segment.metadata.CentralizedDatasourceSchemaConfig; +import org.apache.druid.segment.metadata.SegmentSchemaCache; import org.apache.druid.server.coordinator.simulate.BlockingExecutorService; import org.apache.druid.server.coordinator.simulate.TestDruidLeaderSelector; import org.apache.druid.server.coordinator.simulate.WrappingScheduledExecutorService; @@ -98,9 +99,12 @@ public void setup() emitter = new StubServiceEmitter(); cachePollExecutor = new BlockingExecutorService("test-cache-poll-exec"); segmentMetadataCache = new HeapMemorySegmentMetadataCache( + Set.of(NodeRole.OVERLORD), mapper, () -> new SegmentsMetadataManagerConfig(null, cacheMode), derbyConnectorRule.metadataTablesConfigSupplier(), + () -> CentralizedDatasourceSchemaConfig.enabled(false), + new SegmentSchemaCache(), derbyConnector, (corePoolSize, nameFormat) -> new WrappingScheduledExecutorService( nameFormat, diff --git a/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java b/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java index 96d27bdda325..6655704a9383 100644 --- a/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java +++ b/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java @@ -150,7 +150,9 @@ public void setUp() cachePollExecutor = new BlockingExecutorService("test-cache-poll-exec"); + final Set nodeRoles = Set.of(NodeRole.OVERLORD); segmentMetadataCache = new HeapMemorySegmentMetadataCache( + nodeRoles, mapper, () -> new SegmentsMetadataManagerConfig(null, cacheMode), derbyConnectorRule.metadataTablesConfigSupplier(), @@ -180,7 +182,7 @@ public void setUp() derbyConnectorRule.metadataTablesConfigSupplier().get(), derbyConnector, leaderSelector, - Set.of(NodeRole.OVERLORD), + nodeRoles, segmentMetadataCache, emitter ) diff --git a/server/src/test/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2Test.java b/server/src/test/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2Test.java index 6b02e63498f1..50169ca39a5f 100644 --- a/server/src/test/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2Test.java +++ b/server/src/test/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2Test.java @@ -21,6 +21,7 @@ import com.google.common.base.Suppliers; import org.apache.druid.client.DataSourcesSnapshot; +import org.apache.druid.discovery.NodeRole; import org.apache.druid.java.util.common.DateTimes; import org.apache.druid.java.util.common.granularity.Granularities; import org.apache.druid.java.util.metrics.StubServiceEmitter; @@ -80,12 +81,14 @@ public void setup() throws Exception } private void initManager( + NodeRole nodeRole, SegmentMetadataCache.UsageMode cacheMode, boolean useSchemaCache ) { segmentMetadataCacheExec = new BlockingExecutorService("test"); SegmentMetadataCache segmentMetadataCache = new HeapMemorySegmentMetadataCache( + Set.of(nodeRole), jsonMapper, Suppliers.ofInstance(new SegmentsMetadataManagerConfig(Period.seconds(1), cacheMode)), Suppliers.ofInstance(storageConfig), @@ -133,7 +136,7 @@ public void tearDown() @Test public void test_manager_usesCachedSegments_ifCacheIsEnabled() { - initManager(SegmentMetadataCache.UsageMode.ALWAYS, false); + initManager(NodeRole.OVERLORD, SegmentMetadataCache.UsageMode.ALWAYS, false); manager.startPollingDatabasePeriodically(); Assert.assertTrue(manager.isPollingDatabasePeriodically()); @@ -156,7 +159,7 @@ public void test_manager_usesCachedSegments_ifCacheIsEnabled() @Test public void test_manager_pollsSegments_ifCacheIsDisabled() { - initManager(SegmentMetadataCache.UsageMode.NEVER, false); + initManager(NodeRole.OVERLORD, SegmentMetadataCache.UsageMode.NEVER, false); manager.startPollingDatabasePeriodically(); Assert.assertTrue(manager.isPollingDatabasePeriodically()); @@ -173,7 +176,7 @@ public void test_manager_pollsSegments_ifCacheIsDisabled() @Test public void test_manager_usesCachedSegmentsAndSchemas_ifBothCacheAndSchemaAreEnabled() { - initManager(SegmentMetadataCache.UsageMode.ALWAYS, true); + initManager(NodeRole.COORDINATOR, SegmentMetadataCache.UsageMode.ALWAYS, true); manager.startPollingDatabasePeriodically(); Assert.assertTrue(manager.isPollingDatabasePeriodically()); diff --git a/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java b/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java index f1a5bea26338..7aa49549185a 100644 --- a/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java +++ b/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java @@ -19,6 +19,7 @@ package org.apache.druid.metadata.segment.cache; +import org.apache.druid.discovery.NodeRole; import org.apache.druid.error.DruidExceptionMatcher; import org.apache.druid.java.util.common.DateTimes; import org.apache.druid.java.util.common.ISE; @@ -103,6 +104,7 @@ private void setupTargetWithCaching(SegmentMetadataCache.UsageMode cacheMode) final SegmentsMetadataManagerConfig metadataManagerConfig = new SegmentsMetadataManagerConfig(null, cacheMode); cache = new HeapMemorySegmentMetadataCache( + Set.of(NodeRole.OVERLORD), TestHelper.JSON_MAPPER, () -> metadataManagerConfig, derbyConnectorRule.metadataTablesConfigSupplier(), From 2fcdabffe9afc6f2638a07e7706f663e4b057aa4 Mon Sep 17 00:00:00 2001 From: Kashif Faraz Date: Fri, 16 May 2025 10:52:34 +0530 Subject: [PATCH 06/18] Fix up cache bindings --- .../common/actions/TaskActionTestKit.java | 4 +- .../common/task/IngestionTestBase.java | 4 +- .../guice/SQLMetadataStorageDruidModule.java | 10 +- .../cache/HeapMemorySegmentMetadataCache.java | 53 ++++--- .../metadata/NoopSegmentSchemaCache.java | 132 ++++++++++++++++++ .../segment/metadata/SegmentSchemaCache.java | 8 ++ ...etadataStorageCoordinatorReadOnlyTest.java | 1 - ...exerSQLMetadataStorageCoordinatorTest.java | 4 +- .../SqlSegmentsMetadataManagerV2Test.java | 9 +- .../HeapMemorySegmentMetadataCacheTest.java | 2 - .../org/apache/druid/cli/CliCoordinator.java | 5 + .../org/apache/druid/cli/CliOverlord.java | 11 ++ .../druid/guice/SegmentSchemaCacheModule.java | 2 + 13 files changed, 201 insertions(+), 44 deletions(-) create mode 100644 server/src/main/java/org/apache/druid/segment/metadata/NoopSegmentSchemaCache.java diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/common/actions/TaskActionTestKit.java b/indexing-service/src/test/java/org/apache/druid/indexing/common/actions/TaskActionTestKit.java index b9522cc0f9ee..a5fb2cb04a84 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/common/actions/TaskActionTestKit.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/common/actions/TaskActionTestKit.java @@ -174,9 +174,7 @@ private SqlSegmentMetadataTransactionFactory setupTransactionFactory(ObjectMappe ? SegmentMetadataCache.UsageMode.ALWAYS : SegmentMetadataCache.UsageMode.NEVER; - final Set nodeRoles = Set.of(NodeRole.OVERLORD); segmentMetadataCache = new HeapMemorySegmentMetadataCache( - nodeRoles, objectMapper, Suppliers.ofInstance(new SegmentsMetadataManagerConfig(Period.seconds(1), cacheMode)), Suppliers.ofInstance(metadataStorageTablesConfig), @@ -195,7 +193,7 @@ private SqlSegmentMetadataTransactionFactory setupTransactionFactory(ObjectMappe metadataStorageTablesConfig, testDerbyConnector, leaderSelector, - nodeRoles, + Set.of(NodeRole.OVERLORD), segmentMetadataCache, NoopServiceEmitter.instance() ) diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IngestionTestBase.java b/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IngestionTestBase.java index 04a6f53a2da5..5f6402ce299b 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IngestionTestBase.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IngestionTestBase.java @@ -321,9 +321,7 @@ private SqlSegmentMetadataTransactionFactory createTransactionFactory() = useSegmentMetadataCache ? SegmentMetadataCache.UsageMode.ALWAYS : SegmentMetadataCache.UsageMode.NEVER; - final Set nodeRoles = Set.of(NodeRole.OVERLORD); segmentMetadataCache = new HeapMemorySegmentMetadataCache( - nodeRoles, objectMapper, Suppliers.ofInstance(new SegmentsMetadataManagerConfig(Period.millis(10), cacheMode)), derbyConnectorRule.metadataTablesConfigSupplier(), @@ -342,7 +340,7 @@ private SqlSegmentMetadataTransactionFactory createTransactionFactory() derbyConnectorRule.metadataTablesConfigSupplier().get(), derbyConnectorRule.getConnector(), leaderSelector, - nodeRoles, + Set.of(NodeRole.OVERLORD), segmentMetadataCache, NoopServiceEmitter.instance() ); diff --git a/server/src/main/java/org/apache/druid/guice/SQLMetadataStorageDruidModule.java b/server/src/main/java/org/apache/druid/guice/SQLMetadataStorageDruidModule.java index a884ce01a96c..653a07e7a971 100644 --- a/server/src/main/java/org/apache/druid/guice/SQLMetadataStorageDruidModule.java +++ b/server/src/main/java/org/apache/druid/guice/SQLMetadataStorageDruidModule.java @@ -40,9 +40,7 @@ import org.apache.druid.metadata.SqlSegmentsMetadataManagerProvider; import org.apache.druid.metadata.segment.SegmentMetadataTransactionFactory; import org.apache.druid.metadata.segment.SqlSegmentMetadataTransactionFactory; -import org.apache.druid.metadata.segment.cache.HeapMemorySegmentMetadataCache; import org.apache.druid.metadata.segment.cache.SegmentMetadataCache; -import org.apache.druid.segment.metadata.SegmentSchemaCache; import org.apache.druid.server.audit.AuditManagerConfig; import org.apache.druid.server.audit.AuditSerdeHelper; import org.apache.druid.server.audit.SQLAuditManager; @@ -78,6 +76,7 @@ public void createBindingChoices(Binder binder, String defaultValue) PolyBind.createChoiceWithDefault(binder, prop, Key.get(IndexerMetadataStorageCoordinator.class), defaultValue); PolyBind.createChoiceWithDefault(binder, prop, Key.get(MetadataStorageActionHandlerFactory.class), defaultValue); PolyBind.createChoiceWithDefault(binder, prop, Key.get(MetadataSupervisorManager.class), defaultValue); + PolyBind.createChoiceWithDefault(binder, prop, Key.get(SegmentMetadataCache.class), defaultValue); configureAuditManager(binder); } @@ -100,13 +99,6 @@ public void configure(Binder binder) .to(SQLMetadataRuleManagerProvider.class) .in(LazySingleton.class); - // SegmentMetadataCache is bound for all services but is used only by the Overlord and Coordinator - // similar to some other classes bound here, such as IndexerSQLMetadataStorageCoordinator - binder.bind(SegmentMetadataCache.class) - .to(HeapMemorySegmentMetadataCache.class) - .in(LazySingleton.class); - binder.bind(SegmentSchemaCache.class).in(LazySingleton.class); - PolyBind.optionBinder(binder, Key.get(SegmentMetadataTransactionFactory.class)) .addBinding(type) .to(SqlSegmentMetadataTransactionFactory.class) diff --git a/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java b/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java index fb74a30001ca..03a10b93fac3 100644 --- a/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java +++ b/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java @@ -31,7 +31,6 @@ import com.google.errorprone.annotations.concurrent.GuardedBy; import com.google.inject.Inject; import org.apache.druid.client.DataSourcesSnapshot; -import org.apache.druid.discovery.NodeRole; import org.apache.druid.error.DruidException; import org.apache.druid.error.InternalServerError; import org.apache.druid.java.util.common.DateTimes; @@ -107,6 +106,14 @@ public class HeapMemorySegmentMetadataCache implements SegmentMetadataCache private static final int MIN_SYNC_DELAY_MILLIS = 1000; private static final int MAX_IMMEDIATE_SYNC_RETRIES = 3; + /** + * Delta sync fetches only the updates made after {@code previousSyncStartTime - syncWindow}. + * This duration should be enough to account for transaction retries, server + * clocks not being perfectly synced or any other case that might cause past + * records to appear. + */ + private static final Duration DELTA_SYNC_WINDOW = Duration.standardHours(1); + private enum CacheState { STOPPED, FOLLOWER, LEADER_FIRST_SYNC_PENDING, LEADER_FIRST_SYNC_STARTED, LEADER_READY @@ -143,12 +150,15 @@ private enum CacheState private final ConcurrentHashMap datasourceToSegmentCache = new ConcurrentHashMap<>(); - private final AtomicReference syncFinishTime = new AtomicReference<>(); + /** + * Start time of the previous delta sync. This can be used to roughly identify + * the latest updated timestamp that is present in the cache. + */ + private final AtomicReference previousSyncStartTime = new AtomicReference<>(); private final AtomicReference datasourcesSnapshot = new AtomicReference<>(null); @Inject public HeapMemorySegmentMetadataCache( - Set nodeRoles, ObjectMapper jsonMapper, Supplier config, Supplier tablesConfig, @@ -163,7 +173,7 @@ public HeapMemorySegmentMetadataCache( this.cacheMode = config.get().getCacheUsageMode(); this.pollDuration = config.get().getPollDuration().toStandardDuration(); this.tablesConfig = tablesConfig.get(); - this.useSchemaCache = schemaConfig.get().isEnabled() && nodeRoles.contains(NodeRole.COORDINATOR); + this.useSchemaCache = schemaConfig.get().isEnabled() && segmentSchemaCache.isEnabled(); this.segmentSchemaCache = segmentSchemaCache; this.connector = connector; this.pollExecutor = isEnabled() @@ -218,7 +228,7 @@ public void stop() datasourceToSegmentCache.forEach((datasource, cache) -> cache.stop()); datasourceToSegmentCache.clear(); datasourcesSnapshot.set(null); - syncFinishTime.set(null); + previousSyncStartTime.set(null); updateCacheState(CacheState.STOPPED, "Stopped sync with metadata store"); } @@ -278,9 +288,9 @@ public DataSourcesSnapshot getDataSourcesSnapshot() @Override public void awaitNextSync(long timeoutMillis) { - final DateTime lastSyncTime = syncFinishTime.get(); + final DateTime lastSyncTime = previousSyncStartTime.get(); final Supplier lastSyncTimeIsNotUpdated = - () -> Objects.equals(syncFinishTime.get(), lastSyncTime); + () -> Objects.equals(previousSyncStartTime.get(), lastSyncTime); waitForCacheToFinishSyncWhile(lastSyncTimeIsNotUpdated, timeoutMillis); } @@ -514,15 +524,17 @@ public void onFailure(Throwable t) *

* The following actions are performed in every sync: *

    - *
  • Retrieve all used and unused segment IDs along with their updated timestamps
    • + *
  • Retrieve all used segment IDs along with their updated timestamps.
    • + *
  • Sync segment IDs in the cache with the retrieved segment IDs.
    • *
  • Retrieve payloads of used segments which have been updated in the metadata * store but not in the cache
    • - *
  • Retrieve all pending segments and update the cache as needed
    • - *
  • Remove segments not present in the metadata store
    • - *
  • Reset the max unused partition IDs
    • - *
  • Change the cache state to ready if it is leader and waiting for first sync
    • + *
  • Retrieve all pending segments and update the cache as needed.
    • + *
  • If schema caching is enabled, retrieve segment schemas updated since + * the last sync and update them in the {@link SegmentSchemaCache}.
    • + *
  • Change the cache state to ready if it is leader and waiting for first sync.
    • *
  • Emit metrics
    • *
+ *

* * @return Time taken in milliseconds for the sync to finish */ @@ -543,7 +555,7 @@ private long syncWithMetadataStore() final Map datasourceToSummary = new HashMap<>(); // Fetch all used segments if this is the first sync - if (syncFinishTime.get() == null) { + if (previousSyncStartTime.get() == null) { retrieveAllUsedSegments(datasourceToSummary); } else { retrieveUsedSegmentIds(datasourceToSummary); @@ -555,15 +567,22 @@ private long syncWithMetadataStore() retrieveAllPendingSegments(datasourceToSummary); updatePendingSegmentsInCache(datasourceToSummary, syncStartTime); - if (syncFinishTime.get() == null) { - retrieveUsedSegmentSchemasUpdatedAfter(DateTimes.COMPARE_DATE_AS_STRING_MIN, datasourceToSummary); + final DateTime fetchUpdatesAfter = previousSyncStartTime.get(); + if (fetchUpdatesAfter == null) { + retrieveUsedSegmentSchemasUpdatedAfter( + DateTimes.COMPARE_DATE_AS_STRING_MIN, + datasourceToSummary + ); } else { - retrieveUsedSegmentSchemasUpdatedAfter(syncStartTime, datasourceToSummary); + retrieveUsedSegmentSchemasUpdatedAfter( + fetchUpdatesAfter.minus(DELTA_SYNC_WINDOW), + datasourceToSummary + ); } markCacheSynced(syncStartTime); - syncFinishTime.set(DateTimes.nowUtc()); + previousSyncStartTime.set(syncStartTime); return totalSyncDuration.millisElapsed(); } diff --git a/server/src/main/java/org/apache/druid/segment/metadata/NoopSegmentSchemaCache.java b/server/src/main/java/org/apache/druid/segment/metadata/NoopSegmentSchemaCache.java new file mode 100644 index 000000000000..56dc14e1b91a --- /dev/null +++ b/server/src/main/java/org/apache/druid/segment/metadata/NoopSegmentSchemaCache.java @@ -0,0 +1,132 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.segment.metadata; + +import org.apache.druid.segment.SchemaPayload; +import org.apache.druid.segment.SchemaPayloadPlus; +import org.apache.druid.segment.SegmentMetadata; +import org.apache.druid.timeline.SegmentId; + +import java.util.Map; + +/** + * Noop implementation of {@link SegmentSchemaCache}. + */ +public class NoopSegmentSchemaCache extends SegmentSchemaCache +{ + @Override + public boolean isEnabled() + { + return false; + } + + @Override + public void setInitialized() + { + throw new UnsupportedOperationException(); + } + + @Override + public void onLeaderStop() + { + throw new UnsupportedOperationException(); + } + + @Override + public boolean isInitialized() + { + return false; + } + + @Override + public void awaitInitialization() throws InterruptedException + { + throw new UnsupportedOperationException(); + } + + @Override + public void resetSchemaForPublishedSegments( + Map usedSegmentIdToMetadata, + Map schemaFingerprintToPayload + ) + { + throw new UnsupportedOperationException(); + } + + @Override + public void addRealtimeSegmentSchema(SegmentId segmentId, SchemaPayloadPlus schema) + { + throw new UnsupportedOperationException(); + } + + @Override + public void addSchemaPendingBackfill(SegmentId segmentId, SchemaPayloadPlus schema) + { + throw new UnsupportedOperationException(); + } + + @Override + public void markSchemaPersisted(SegmentId segmentId) + { + throw new UnsupportedOperationException(); + } + + @Override + public Map getSegmentMetadataMap() + { + throw new UnsupportedOperationException(); + } + + @Override + public boolean isSchemaCached(SegmentId segmentId) + { + throw new UnsupportedOperationException(); + } + + @Override + public Map getSchemaPayloadMap() + { + throw new UnsupportedOperationException(); + } + + @Override + SchemaPayloadPlus getTemporaryPublishedMetadataQueryResults(SegmentId id) + { + throw new UnsupportedOperationException(); + } + + @Override + public void segmentRemoved(SegmentId segmentId) + { + throw new UnsupportedOperationException(); + } + + @Override + public void realtimeSegmentRemoved(SegmentId segmentId) + { + throw new UnsupportedOperationException(); + } + + @Override + public Map getStats() + { + throw new UnsupportedOperationException(); + } +} diff --git a/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaCache.java b/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaCache.java index 4e86593eb7be..2d46e601afd5 100644 --- a/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaCache.java +++ b/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaCache.java @@ -94,6 +94,14 @@ public class SegmentSchemaCache */ private final AtomicInteger cacheMissCount = new AtomicInteger(0); + /** + * @return true if this cache is enabled. + */ + public boolean isEnabled() + { + return true; + } + public void setInitialized() { if (!isInitialized()) { diff --git a/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorReadOnlyTest.java b/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorReadOnlyTest.java index 8ab0d3d5ee7e..ada0ed1c887b 100644 --- a/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorReadOnlyTest.java +++ b/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorReadOnlyTest.java @@ -99,7 +99,6 @@ public void setup() emitter = new StubServiceEmitter(); cachePollExecutor = new BlockingExecutorService("test-cache-poll-exec"); segmentMetadataCache = new HeapMemorySegmentMetadataCache( - Set.of(NodeRole.OVERLORD), mapper, () -> new SegmentsMetadataManagerConfig(null, cacheMode), derbyConnectorRule.metadataTablesConfigSupplier(), diff --git a/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java b/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java index 6655704a9383..96d27bdda325 100644 --- a/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java +++ b/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java @@ -150,9 +150,7 @@ public void setUp() cachePollExecutor = new BlockingExecutorService("test-cache-poll-exec"); - final Set nodeRoles = Set.of(NodeRole.OVERLORD); segmentMetadataCache = new HeapMemorySegmentMetadataCache( - nodeRoles, mapper, () -> new SegmentsMetadataManagerConfig(null, cacheMode), derbyConnectorRule.metadataTablesConfigSupplier(), @@ -182,7 +180,7 @@ public void setUp() derbyConnectorRule.metadataTablesConfigSupplier().get(), derbyConnector, leaderSelector, - nodeRoles, + Set.of(NodeRole.OVERLORD), segmentMetadataCache, emitter ) diff --git a/server/src/test/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2Test.java b/server/src/test/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2Test.java index 50169ca39a5f..6b02e63498f1 100644 --- a/server/src/test/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2Test.java +++ b/server/src/test/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2Test.java @@ -21,7 +21,6 @@ import com.google.common.base.Suppliers; import org.apache.druid.client.DataSourcesSnapshot; -import org.apache.druid.discovery.NodeRole; import org.apache.druid.java.util.common.DateTimes; import org.apache.druid.java.util.common.granularity.Granularities; import org.apache.druid.java.util.metrics.StubServiceEmitter; @@ -81,14 +80,12 @@ public void setup() throws Exception } private void initManager( - NodeRole nodeRole, SegmentMetadataCache.UsageMode cacheMode, boolean useSchemaCache ) { segmentMetadataCacheExec = new BlockingExecutorService("test"); SegmentMetadataCache segmentMetadataCache = new HeapMemorySegmentMetadataCache( - Set.of(nodeRole), jsonMapper, Suppliers.ofInstance(new SegmentsMetadataManagerConfig(Period.seconds(1), cacheMode)), Suppliers.ofInstance(storageConfig), @@ -136,7 +133,7 @@ public void tearDown() @Test public void test_manager_usesCachedSegments_ifCacheIsEnabled() { - initManager(NodeRole.OVERLORD, SegmentMetadataCache.UsageMode.ALWAYS, false); + initManager(SegmentMetadataCache.UsageMode.ALWAYS, false); manager.startPollingDatabasePeriodically(); Assert.assertTrue(manager.isPollingDatabasePeriodically()); @@ -159,7 +156,7 @@ public void test_manager_usesCachedSegments_ifCacheIsEnabled() @Test public void test_manager_pollsSegments_ifCacheIsDisabled() { - initManager(NodeRole.OVERLORD, SegmentMetadataCache.UsageMode.NEVER, false); + initManager(SegmentMetadataCache.UsageMode.NEVER, false); manager.startPollingDatabasePeriodically(); Assert.assertTrue(manager.isPollingDatabasePeriodically()); @@ -176,7 +173,7 @@ public void test_manager_pollsSegments_ifCacheIsDisabled() @Test public void test_manager_usesCachedSegmentsAndSchemas_ifBothCacheAndSchemaAreEnabled() { - initManager(NodeRole.COORDINATOR, SegmentMetadataCache.UsageMode.ALWAYS, true); + initManager(SegmentMetadataCache.UsageMode.ALWAYS, true); manager.startPollingDatabasePeriodically(); Assert.assertTrue(manager.isPollingDatabasePeriodically()); diff --git a/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java b/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java index 7aa49549185a..f1a5bea26338 100644 --- a/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java +++ b/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java @@ -19,7 +19,6 @@ package org.apache.druid.metadata.segment.cache; -import org.apache.druid.discovery.NodeRole; import org.apache.druid.error.DruidExceptionMatcher; import org.apache.druid.java.util.common.DateTimes; import org.apache.druid.java.util.common.ISE; @@ -104,7 +103,6 @@ private void setupTargetWithCaching(SegmentMetadataCache.UsageMode cacheMode) final SegmentsMetadataManagerConfig metadataManagerConfig = new SegmentsMetadataManagerConfig(null, cacheMode); cache = new HeapMemorySegmentMetadataCache( - Set.of(NodeRole.OVERLORD), TestHelper.JSON_MAPPER, () -> metadataManagerConfig, derbyConnectorRule.metadataTablesConfigSupplier(), diff --git a/services/src/main/java/org/apache/druid/cli/CliCoordinator.java b/services/src/main/java/org/apache/druid/cli/CliCoordinator.java index 4adf29d62387..46fa00f67bad 100644 --- a/services/src/main/java/org/apache/druid/cli/CliCoordinator.java +++ b/services/src/main/java/org/apache/druid/cli/CliCoordinator.java @@ -67,6 +67,8 @@ import org.apache.druid.metadata.MetadataStorageProvider; import org.apache.druid.metadata.SegmentsMetadataManager; import org.apache.druid.metadata.SegmentsMetadataManagerProvider; +import org.apache.druid.metadata.segment.cache.HeapMemorySegmentMetadataCache; +import org.apache.druid.metadata.segment.cache.SegmentMetadataCache; import org.apache.druid.query.lookup.LookupSerdeModule; import org.apache.druid.segment.metadata.CoordinatorSegmentMetadataCache; import org.apache.druid.segment.metadata.SegmentMetadataCacheConfig; @@ -216,6 +218,9 @@ public void configure(Binder binder) binder.bind(SegmentsMetadataManager.class) .toProvider(SegmentsMetadataManagerProvider.class) .in(ManageLifecycle.class); + binder.bind(SegmentMetadataCache.class) + .to(HeapMemorySegmentMetadataCache.class) + .in(LazySingleton.class); binder.bind(MetadataRuleManager.class) .toProvider(MetadataRuleManagerProvider.class) diff --git a/services/src/main/java/org/apache/druid/cli/CliOverlord.java b/services/src/main/java/org/apache/druid/cli/CliOverlord.java index 5cdf7fefd3ec..71dabdde6df1 100644 --- a/services/src/main/java/org/apache/druid/cli/CliOverlord.java +++ b/services/src/main/java/org/apache/druid/cli/CliOverlord.java @@ -111,8 +111,12 @@ import org.apache.druid.metadata.SegmentsMetadataManager; import org.apache.druid.metadata.SegmentsMetadataManagerProvider; import org.apache.druid.metadata.input.InputSourceModule; +import org.apache.druid.metadata.segment.cache.HeapMemorySegmentMetadataCache; +import org.apache.druid.metadata.segment.cache.SegmentMetadataCache; import org.apache.druid.query.lookup.LookupSerdeModule; import org.apache.druid.segment.incremental.RowIngestionMetersFactory; +import org.apache.druid.segment.metadata.NoopSegmentSchemaCache; +import org.apache.druid.segment.metadata.SegmentSchemaCache; import org.apache.druid.segment.realtime.ChatHandlerProvider; import org.apache.druid.segment.realtime.NoopChatHandlerProvider; import org.apache.druid.segment.realtime.appenderator.AppenderatorsManager; @@ -215,6 +219,13 @@ public void configure(Binder binder) .toProvider(SegmentsMetadataManagerProvider.class) .in(ManageLifecycle.class); binder.bind(CoordinatorConfigManager.class).in(LazySingleton.class); + + binder.bind(SegmentMetadataCache.class) + .to(HeapMemorySegmentMetadataCache.class) + .in(LazySingleton.class); + binder.bind(SegmentSchemaCache.class) + .to(NoopSegmentSchemaCache.class) + .in(LazySingleton.class); } JsonConfigProvider.bind(binder, "druid.coordinator.asOverlord", CoordinatorOverlordServiceConfig.class); diff --git a/services/src/main/java/org/apache/druid/guice/SegmentSchemaCacheModule.java b/services/src/main/java/org/apache/druid/guice/SegmentSchemaCacheModule.java index 744d56a89e02..859abc76c8b7 100644 --- a/services/src/main/java/org/apache/druid/guice/SegmentSchemaCacheModule.java +++ b/services/src/main/java/org/apache/druid/guice/SegmentSchemaCacheModule.java @@ -44,6 +44,7 @@ import org.apache.druid.segment.metadata.CentralizedDatasourceSchemaConfig; import org.apache.druid.segment.metadata.CoordinatorSegmentMetadataCache; import org.apache.druid.segment.metadata.SegmentMetadataQuerySegmentWalker; +import org.apache.druid.segment.metadata.SegmentSchemaCache; import org.apache.druid.server.QueryScheduler; import org.apache.druid.server.QuerySchedulerProvider; @@ -83,6 +84,7 @@ public void configure(Binder binder) .in(LazySingleton.class); binder.bind(QuerySchedulerProvider.class).in(LazySingleton.class); binder.bind(QuerySegmentWalker.class).to(SegmentMetadataQuerySegmentWalker.class).in(LazySingleton.class); + binder.bind(SegmentSchemaCache.class).in(LazySingleton.class); LifecycleModule.register(binder, CoordinatorSegmentMetadataCache.class); } From 4c65101d79c249bdaa7b767e528bcb2a201b46f5 Mon Sep 17 00:00:00 2001 From: Kashif Faraz Date: Fri, 16 May 2025 10:58:26 +0530 Subject: [PATCH 07/18] Remove extra binding --- .../org/apache/druid/guice/SQLMetadataStorageDruidModule.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/server/src/main/java/org/apache/druid/guice/SQLMetadataStorageDruidModule.java b/server/src/main/java/org/apache/druid/guice/SQLMetadataStorageDruidModule.java index 653a07e7a971..92c0118547bf 100644 --- a/server/src/main/java/org/apache/druid/guice/SQLMetadataStorageDruidModule.java +++ b/server/src/main/java/org/apache/druid/guice/SQLMetadataStorageDruidModule.java @@ -40,7 +40,6 @@ import org.apache.druid.metadata.SqlSegmentsMetadataManagerProvider; import org.apache.druid.metadata.segment.SegmentMetadataTransactionFactory; import org.apache.druid.metadata.segment.SqlSegmentMetadataTransactionFactory; -import org.apache.druid.metadata.segment.cache.SegmentMetadataCache; import org.apache.druid.server.audit.AuditManagerConfig; import org.apache.druid.server.audit.AuditSerdeHelper; import org.apache.druid.server.audit.SQLAuditManager; @@ -76,7 +75,6 @@ public void createBindingChoices(Binder binder, String defaultValue) PolyBind.createChoiceWithDefault(binder, prop, Key.get(IndexerMetadataStorageCoordinator.class), defaultValue); PolyBind.createChoiceWithDefault(binder, prop, Key.get(MetadataStorageActionHandlerFactory.class), defaultValue); PolyBind.createChoiceWithDefault(binder, prop, Key.get(MetadataSupervisorManager.class), defaultValue); - PolyBind.createChoiceWithDefault(binder, prop, Key.get(SegmentMetadataCache.class), defaultValue); configureAuditManager(binder); } From daa8e277b045e9273a1a4a4de28e2aa842bbd47f Mon Sep 17 00:00:00 2001 From: Kashif Faraz Date: Fri, 16 May 2025 13:21:45 +0530 Subject: [PATCH 08/18] More dependency stuff --- .../SegmentMetadataTransactionFactory.java | 25 ++++++++++ .../java/org/apache/druid/cli/CliBroker.java | 4 +- .../org/apache/druid/cli/CliHistorical.java | 4 +- .../java/org/apache/druid/cli/CliIndexer.java | 4 +- .../apache/druid/cli/CliMiddleManager.java | 4 +- .../java/org/apache/druid/cli/CliPeon.java | 4 +- .../java/org/apache/druid/cli/CliRouter.java | 4 +- .../guice/NoopSegmentMetadataCacheModule.java | 47 +++++++++++++++++++ 8 files changed, 90 insertions(+), 6 deletions(-) create mode 100644 services/src/main/java/org/apache/druid/guice/NoopSegmentMetadataCacheModule.java diff --git a/server/src/main/java/org/apache/druid/metadata/segment/SegmentMetadataTransactionFactory.java b/server/src/main/java/org/apache/druid/metadata/segment/SegmentMetadataTransactionFactory.java index e048e10970c6..6bfbf564c964 100644 --- a/server/src/main/java/org/apache/druid/metadata/segment/SegmentMetadataTransactionFactory.java +++ b/server/src/main/java/org/apache/druid/metadata/segment/SegmentMetadataTransactionFactory.java @@ -41,4 +41,29 @@ T inReadWriteDatasourceTransaction( String dataSource, SegmentMetadataTransaction.Callback callback ); + + /** + * No-op instance of {@link SegmentMetadataTransactionFactory} which does not + * support any operation. + */ + SegmentMetadataTransactionFactory NOOP = new SegmentMetadataTransactionFactory() + { + @Override + public T inReadOnlyDatasourceTransaction( + String dataSource, + SegmentMetadataReadTransaction.Callback callback + ) + { + throw new UnsupportedOperationException(); + } + + @Override + public T inReadWriteDatasourceTransaction( + String dataSource, + SegmentMetadataTransaction.Callback callback + ) + { + throw new UnsupportedOperationException(); + } + }; } diff --git a/services/src/main/java/org/apache/druid/cli/CliBroker.java b/services/src/main/java/org/apache/druid/cli/CliBroker.java index 7af834942bdf..cdef522c4982 100644 --- a/services/src/main/java/org/apache/druid/cli/CliBroker.java +++ b/services/src/main/java/org/apache/druid/cli/CliBroker.java @@ -50,6 +50,7 @@ import org.apache.druid.guice.LazySingleton; import org.apache.druid.guice.LifecycleModule; import org.apache.druid.guice.ManageLifecycle; +import org.apache.druid.guice.NoopSegmentMetadataCacheModule; import org.apache.druid.guice.QueryRunnerFactoryModule; import org.apache.druid.guice.QueryableModule; import org.apache.druid.guice.SegmentWranglerModule; @@ -194,7 +195,8 @@ protected List getModules() .in(LazySingleton.class); }, new LookupModule(), - new SqlModule() + new SqlModule(), + new NoopSegmentMetadataCacheModule() ); } } diff --git a/services/src/main/java/org/apache/druid/cli/CliHistorical.java b/services/src/main/java/org/apache/druid/cli/CliHistorical.java index 433d9ced54ef..ce3f9f6bd024 100644 --- a/services/src/main/java/org/apache/druid/cli/CliHistorical.java +++ b/services/src/main/java/org/apache/druid/cli/CliHistorical.java @@ -40,6 +40,7 @@ import org.apache.druid.guice.LazySingleton; import org.apache.druid.guice.LifecycleModule; import org.apache.druid.guice.ManageLifecycle; +import org.apache.druid.guice.NoopSegmentMetadataCacheModule; import org.apache.druid.guice.QueryRunnerFactoryModule; import org.apache.druid.guice.QueryableModule; import org.apache.druid.guice.SegmentWranglerModule; @@ -146,7 +147,8 @@ protected List getModules() .toProvider(new LocalTmpStorageConfig.DefaultLocalTmpStorageConfigProvider("historical")) .in(LazySingleton.class); }, - new LookupModule() + new LookupModule(), + new NoopSegmentMetadataCacheModule() ); } diff --git a/services/src/main/java/org/apache/druid/cli/CliIndexer.java b/services/src/main/java/org/apache/druid/cli/CliIndexer.java index 46e243e71bec..c473fcc76a4e 100644 --- a/services/src/main/java/org/apache/druid/cli/CliIndexer.java +++ b/services/src/main/java/org/apache/druid/cli/CliIndexer.java @@ -46,6 +46,7 @@ import org.apache.druid.guice.LazySingleton; import org.apache.druid.guice.LifecycleModule; import org.apache.druid.guice.ManageLifecycle; +import org.apache.druid.guice.NoopSegmentMetadataCacheModule; import org.apache.druid.guice.QueryRunnerFactoryModule; import org.apache.druid.guice.QueryableModule; import org.apache.druid.guice.QueryablePeonModule; @@ -243,7 +244,8 @@ public DataNodeService getDataNodeService(DruidServerConfig serverConfig) new InputSourceModule(), new QueryablePeonModule(), new CliIndexerServerModule(properties), - new LookupModule() + new LookupModule(), + new NoopSegmentMetadataCacheModule() ); } } diff --git a/services/src/main/java/org/apache/druid/cli/CliMiddleManager.java b/services/src/main/java/org/apache/druid/cli/CliMiddleManager.java index 0b2a8d02ad8d..657020c26ed0 100644 --- a/services/src/main/java/org/apache/druid/cli/CliMiddleManager.java +++ b/services/src/main/java/org/apache/druid/cli/CliMiddleManager.java @@ -46,6 +46,7 @@ import org.apache.druid.guice.LifecycleModule; import org.apache.druid.guice.ManageLifecycle; import org.apache.druid.guice.MiddleManagerServiceModule; +import org.apache.druid.guice.NoopSegmentMetadataCacheModule; import org.apache.druid.guice.PolyBind; import org.apache.druid.guice.annotations.Self; import org.apache.druid.indexing.common.RetryPolicyFactory; @@ -250,7 +251,8 @@ public WorkerNodeService getWorkerNodeService(WorkerConfig workerConfig) new IndexingServiceTaskLogsModule(), new IndexingServiceTuningConfigModule(), new InputSourceModule(), - new LookupSerdeModule() + new LookupSerdeModule(), + new NoopSegmentMetadataCacheModule() ); } diff --git a/services/src/main/java/org/apache/druid/cli/CliPeon.java b/services/src/main/java/org/apache/druid/cli/CliPeon.java index 065a6430563f..73a51f9896ae 100644 --- a/services/src/main/java/org/apache/druid/cli/CliPeon.java +++ b/services/src/main/java/org/apache/druid/cli/CliPeon.java @@ -57,6 +57,7 @@ import org.apache.druid.guice.LifecycleModule; import org.apache.druid.guice.ManageLifecycle; import org.apache.druid.guice.ManageLifecycleServer; +import org.apache.druid.guice.NoopSegmentMetadataCacheModule; import org.apache.druid.guice.PeonProcessingModule; import org.apache.druid.guice.PolyBind; import org.apache.druid.guice.QueryRunnerFactoryModule; @@ -383,7 +384,8 @@ public LocalTmpStorageConfig getLocalTmpStorage() new IndexingServiceTuningConfigModule(), new InputSourceModule(), new ChatHandlerServerModule(properties), - new LookupModule() + new LookupModule(), + new NoopSegmentMetadataCacheModule() ); } diff --git a/services/src/main/java/org/apache/druid/cli/CliRouter.java b/services/src/main/java/org/apache/druid/cli/CliRouter.java index a3344aa4e7fe..66e81e3e789a 100644 --- a/services/src/main/java/org/apache/druid/cli/CliRouter.java +++ b/services/src/main/java/org/apache/druid/cli/CliRouter.java @@ -33,6 +33,7 @@ import org.apache.druid.guice.LazySingleton; import org.apache.druid.guice.LifecycleModule; import org.apache.druid.guice.ManageLifecycle; +import org.apache.druid.guice.NoopSegmentMetadataCacheModule; import org.apache.druid.guice.QueryRunnerFactoryModule; import org.apache.druid.guice.QueryableModule; import org.apache.druid.guice.RouterProcessingModule; @@ -132,7 +133,8 @@ protected List getModules() .toProvider(new LocalTmpStorageConfig.DefaultLocalTmpStorageConfigProvider("router")) .in(LazySingleton.class); }, - new LookupSerdeModule() + new LookupSerdeModule(), + new NoopSegmentMetadataCacheModule() ); } } diff --git a/services/src/main/java/org/apache/druid/guice/NoopSegmentMetadataCacheModule.java b/services/src/main/java/org/apache/druid/guice/NoopSegmentMetadataCacheModule.java new file mode 100644 index 000000000000..d9e1e2bf1548 --- /dev/null +++ b/services/src/main/java/org/apache/druid/guice/NoopSegmentMetadataCacheModule.java @@ -0,0 +1,47 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.guice; + +import com.google.inject.Binder; +import com.google.inject.Module; +import org.apache.druid.metadata.segment.cache.NoopSegmentMetadataCache; +import org.apache.druid.metadata.segment.cache.SegmentMetadataCache; +import org.apache.druid.segment.metadata.NoopSegmentSchemaCache; +import org.apache.druid.segment.metadata.SegmentSchemaCache; + +/** + * Module used by services other than Overlord and Coordinator to bind + * {@link SegmentMetadataCache} and {@link SegmentSchemaCache} to noop instances. + *

+ * Classes using these caches like {@code SqlSegmentMetadataTransactionFactory} + * and {@code SqlSegmentsMetadataManagerProvider} are currently bound for all + * services in {@code SQLMetadataStorageDruidModule}. Ideally, this module should + * be installed only for Coordinator/Overlord since other services do not access + * the metadata store directly. + */ +public class NoopSegmentMetadataCacheModule implements Module +{ + @Override + public void configure(Binder binder) + { + binder.bind(SegmentMetadataCache.class).to(NoopSegmentMetadataCache.class).in(LazySingleton.class); + binder.bind(SegmentSchemaCache.class).to(NoopSegmentSchemaCache.class).in(LazySingleton.class); + } +} From 23bbd9af8af54f060a12599c308d4763c1691581 Mon Sep 17 00:00:00 2001 From: Kashif Faraz Date: Sat, 17 May 2025 01:09:37 +0530 Subject: [PATCH 09/18] Fix up guice bindings for real --- .../guice/SQLMetadataStorageDruidModule.java | 48 -------- .../java/org/apache/druid/cli/CliBroker.java | 4 +- .../org/apache/druid/cli/CliCoordinator.java | 22 +--- .../org/apache/druid/cli/CliHistorical.java | 4 +- .../java/org/apache/druid/cli/CliIndexer.java | 4 +- .../apache/druid/cli/CliMiddleManager.java | 4 +- .../org/apache/druid/cli/CliOverlord.java | 20 +--- .../java/org/apache/druid/cli/CliPeon.java | 11 +- .../java/org/apache/druid/cli/CliRouter.java | 4 +- .../druid/guice/MetadataManagerModule.java | 108 ++++++++++++++++++ .../guice/NoopSegmentMetadataCacheModule.java | 47 -------- .../druid/guice/SegmentSchemaCacheModule.java | 2 - 12 files changed, 120 insertions(+), 158 deletions(-) create mode 100644 services/src/main/java/org/apache/druid/guice/MetadataManagerModule.java delete mode 100644 services/src/main/java/org/apache/druid/guice/NoopSegmentMetadataCacheModule.java diff --git a/server/src/main/java/org/apache/druid/guice/SQLMetadataStorageDruidModule.java b/server/src/main/java/org/apache/druid/guice/SQLMetadataStorageDruidModule.java index 92c0118547bf..8ef0f36a07fa 100644 --- a/server/src/main/java/org/apache/druid/guice/SQLMetadataStorageDruidModule.java +++ b/server/src/main/java/org/apache/druid/guice/SQLMetadataStorageDruidModule.java @@ -23,23 +23,10 @@ import com.google.inject.Key; import com.google.inject.Module; import org.apache.druid.audit.AuditManager; -import org.apache.druid.indexing.overlord.IndexerMetadataStorageCoordinator; -import org.apache.druid.metadata.IndexerSQLMetadataStorageCoordinator; -import org.apache.druid.metadata.MetadataRuleManager; -import org.apache.druid.metadata.MetadataRuleManagerProvider; import org.apache.druid.metadata.MetadataStorageActionHandlerFactory; import org.apache.druid.metadata.MetadataStorageConnector; import org.apache.druid.metadata.MetadataStorageProvider; -import org.apache.druid.metadata.MetadataSupervisorManager; import org.apache.druid.metadata.SQLMetadataConnector; -import org.apache.druid.metadata.SQLMetadataRuleManager; -import org.apache.druid.metadata.SQLMetadataRuleManagerProvider; -import org.apache.druid.metadata.SQLMetadataSupervisorManager; -import org.apache.druid.metadata.SegmentsMetadataManager; -import org.apache.druid.metadata.SegmentsMetadataManagerProvider; -import org.apache.druid.metadata.SqlSegmentsMetadataManagerProvider; -import org.apache.druid.metadata.segment.SegmentMetadataTransactionFactory; -import org.apache.druid.metadata.segment.SqlSegmentMetadataTransactionFactory; import org.apache.druid.server.audit.AuditManagerConfig; import org.apache.druid.server.audit.AuditSerdeHelper; import org.apache.druid.server.audit.SQLAuditManager; @@ -67,14 +54,7 @@ public void createBindingChoices(Binder binder, String defaultValue) PolyBind.createChoiceWithDefault(binder, prop, Key.get(MetadataStorageProvider.class), defaultValue); PolyBind.createChoiceWithDefault(binder, prop, Key.get(SQLMetadataConnector.class), defaultValue); - PolyBind.createChoiceWithDefault(binder, prop, Key.get(SegmentsMetadataManager.class), defaultValue); - PolyBind.createChoiceWithDefault(binder, prop, Key.get(SegmentsMetadataManagerProvider.class), defaultValue); - PolyBind.createChoiceWithDefault(binder, prop, Key.get(MetadataRuleManager.class), defaultValue); - PolyBind.createChoiceWithDefault(binder, prop, Key.get(MetadataRuleManagerProvider.class), defaultValue); - PolyBind.createChoiceWithDefault(binder, prop, Key.get(SegmentMetadataTransactionFactory.class), defaultValue); - PolyBind.createChoiceWithDefault(binder, prop, Key.get(IndexerMetadataStorageCoordinator.class), defaultValue); PolyBind.createChoiceWithDefault(binder, prop, Key.get(MetadataStorageActionHandlerFactory.class), defaultValue); - PolyBind.createChoiceWithDefault(binder, prop, Key.get(MetadataSupervisorManager.class), defaultValue); configureAuditManager(binder); } @@ -82,35 +62,7 @@ public void createBindingChoices(Binder binder, String defaultValue) @Override public void configure(Binder binder) { - PolyBind.optionBinder(binder, Key.get(SegmentsMetadataManagerProvider.class)) - .addBinding(type) - .to(SqlSegmentsMetadataManagerProvider.class) - .in(LazySingleton.class); - PolyBind.optionBinder(binder, Key.get(MetadataRuleManager.class)) - .addBinding(type) - .to(SQLMetadataRuleManager.class) - .in(LazySingleton.class); - - PolyBind.optionBinder(binder, Key.get(MetadataRuleManagerProvider.class)) - .addBinding(type) - .to(SQLMetadataRuleManagerProvider.class) - .in(LazySingleton.class); - - PolyBind.optionBinder(binder, Key.get(SegmentMetadataTransactionFactory.class)) - .addBinding(type) - .to(SqlSegmentMetadataTransactionFactory.class) - .in(LazySingleton.class); - - PolyBind.optionBinder(binder, Key.get(IndexerMetadataStorageCoordinator.class)) - .addBinding(type) - .to(IndexerSQLMetadataStorageCoordinator.class) - .in(ManageLifecycle.class); - - PolyBind.optionBinder(binder, Key.get(MetadataSupervisorManager.class)) - .addBinding(type) - .to(SQLMetadataSupervisorManager.class) - .in(LazySingleton.class); } private void configureAuditManager(Binder binder) diff --git a/services/src/main/java/org/apache/druid/cli/CliBroker.java b/services/src/main/java/org/apache/druid/cli/CliBroker.java index cdef522c4982..7af834942bdf 100644 --- a/services/src/main/java/org/apache/druid/cli/CliBroker.java +++ b/services/src/main/java/org/apache/druid/cli/CliBroker.java @@ -50,7 +50,6 @@ import org.apache.druid.guice.LazySingleton; import org.apache.druid.guice.LifecycleModule; import org.apache.druid.guice.ManageLifecycle; -import org.apache.druid.guice.NoopSegmentMetadataCacheModule; import org.apache.druid.guice.QueryRunnerFactoryModule; import org.apache.druid.guice.QueryableModule; import org.apache.druid.guice.SegmentWranglerModule; @@ -195,8 +194,7 @@ protected List getModules() .in(LazySingleton.class); }, new LookupModule(), - new SqlModule(), - new NoopSegmentMetadataCacheModule() + new SqlModule() ); } } diff --git a/services/src/main/java/org/apache/druid/cli/CliCoordinator.java b/services/src/main/java/org/apache/druid/cli/CliCoordinator.java index 46fa00f67bad..ffe06dda70a4 100644 --- a/services/src/main/java/org/apache/druid/cli/CliCoordinator.java +++ b/services/src/main/java/org/apache/druid/cli/CliCoordinator.java @@ -47,6 +47,7 @@ import org.apache.druid.guice.LazySingleton; import org.apache.druid.guice.LifecycleModule; import org.apache.druid.guice.ManageLifecycle; +import org.apache.druid.guice.MetadataManagerModule; import org.apache.druid.guice.QueryableModule; import org.apache.druid.guice.SegmentSchemaCacheModule; import org.apache.druid.guice.SupervisorCleanupModule; @@ -61,14 +62,8 @@ import org.apache.druid.java.util.common.lifecycle.Lifecycle; import org.apache.druid.java.util.common.logger.Logger; import org.apache.druid.java.util.http.client.HttpClient; -import org.apache.druid.metadata.MetadataRuleManager; -import org.apache.druid.metadata.MetadataRuleManagerProvider; import org.apache.druid.metadata.MetadataStorage; import org.apache.druid.metadata.MetadataStorageProvider; -import org.apache.druid.metadata.SegmentsMetadataManager; -import org.apache.druid.metadata.SegmentsMetadataManagerProvider; -import org.apache.druid.metadata.segment.cache.HeapMemorySegmentMetadataCache; -import org.apache.druid.metadata.segment.cache.SegmentMetadataCache; import org.apache.druid.query.lookup.LookupSerdeModule; import org.apache.druid.segment.metadata.CoordinatorSegmentMetadataCache; import org.apache.druid.segment.metadata.SegmentMetadataCacheConfig; @@ -76,7 +71,6 @@ import org.apache.druid.server.coordinator.CloneStatusManager; import org.apache.druid.server.coordinator.CoordinatorConfigManager; import org.apache.druid.server.coordinator.DruidCoordinator; -import org.apache.druid.server.coordinator.MetadataManager; import org.apache.druid.server.coordinator.balancer.BalancerStrategyFactory; import org.apache.druid.server.coordinator.config.CoordinatorKillConfigs; import org.apache.druid.server.coordinator.config.CoordinatorPeriodConfig; @@ -169,6 +163,7 @@ protected List getModules() List modules = new ArrayList<>(); modules.add(JettyHttpClientModule.global()); + modules.add(new MetadataManagerModule()); if (isSegmentSchemaCacheEnabled) { validateCentralizedDatasourceSchemaConfig(properties); @@ -215,22 +210,9 @@ public void configure(Binder binder) binder.bind(DirectDruidClientFactory.class).toProvider(Providers.of(null)); } - binder.bind(SegmentsMetadataManager.class) - .toProvider(SegmentsMetadataManagerProvider.class) - .in(ManageLifecycle.class); - binder.bind(SegmentMetadataCache.class) - .to(HeapMemorySegmentMetadataCache.class) - .in(LazySingleton.class); - - binder.bind(MetadataRuleManager.class) - .toProvider(MetadataRuleManagerProvider.class) - .in(ManageLifecycle.class); - binder.bind(LookupCoordinatorManager.class).in(LazySingleton.class); binder.bind(CloneStatusManager.class).in(LazySingleton.class); - binder.bind(CoordinatorConfigManager.class); - binder.bind(MetadataManager.class); binder.bind(DruidCoordinator.class); binder.bind(CompactionStatusTracker.class).in(LazySingleton.class); diff --git a/services/src/main/java/org/apache/druid/cli/CliHistorical.java b/services/src/main/java/org/apache/druid/cli/CliHistorical.java index ce3f9f6bd024..433d9ced54ef 100644 --- a/services/src/main/java/org/apache/druid/cli/CliHistorical.java +++ b/services/src/main/java/org/apache/druid/cli/CliHistorical.java @@ -40,7 +40,6 @@ import org.apache.druid.guice.LazySingleton; import org.apache.druid.guice.LifecycleModule; import org.apache.druid.guice.ManageLifecycle; -import org.apache.druid.guice.NoopSegmentMetadataCacheModule; import org.apache.druid.guice.QueryRunnerFactoryModule; import org.apache.druid.guice.QueryableModule; import org.apache.druid.guice.SegmentWranglerModule; @@ -147,8 +146,7 @@ protected List getModules() .toProvider(new LocalTmpStorageConfig.DefaultLocalTmpStorageConfigProvider("historical")) .in(LazySingleton.class); }, - new LookupModule(), - new NoopSegmentMetadataCacheModule() + new LookupModule() ); } diff --git a/services/src/main/java/org/apache/druid/cli/CliIndexer.java b/services/src/main/java/org/apache/druid/cli/CliIndexer.java index c473fcc76a4e..46e243e71bec 100644 --- a/services/src/main/java/org/apache/druid/cli/CliIndexer.java +++ b/services/src/main/java/org/apache/druid/cli/CliIndexer.java @@ -46,7 +46,6 @@ import org.apache.druid.guice.LazySingleton; import org.apache.druid.guice.LifecycleModule; import org.apache.druid.guice.ManageLifecycle; -import org.apache.druid.guice.NoopSegmentMetadataCacheModule; import org.apache.druid.guice.QueryRunnerFactoryModule; import org.apache.druid.guice.QueryableModule; import org.apache.druid.guice.QueryablePeonModule; @@ -244,8 +243,7 @@ public DataNodeService getDataNodeService(DruidServerConfig serverConfig) new InputSourceModule(), new QueryablePeonModule(), new CliIndexerServerModule(properties), - new LookupModule(), - new NoopSegmentMetadataCacheModule() + new LookupModule() ); } } diff --git a/services/src/main/java/org/apache/druid/cli/CliMiddleManager.java b/services/src/main/java/org/apache/druid/cli/CliMiddleManager.java index 657020c26ed0..0b2a8d02ad8d 100644 --- a/services/src/main/java/org/apache/druid/cli/CliMiddleManager.java +++ b/services/src/main/java/org/apache/druid/cli/CliMiddleManager.java @@ -46,7 +46,6 @@ import org.apache.druid.guice.LifecycleModule; import org.apache.druid.guice.ManageLifecycle; import org.apache.druid.guice.MiddleManagerServiceModule; -import org.apache.druid.guice.NoopSegmentMetadataCacheModule; import org.apache.druid.guice.PolyBind; import org.apache.druid.guice.annotations.Self; import org.apache.druid.indexing.common.RetryPolicyFactory; @@ -251,8 +250,7 @@ public WorkerNodeService getWorkerNodeService(WorkerConfig workerConfig) new IndexingServiceTaskLogsModule(), new IndexingServiceTuningConfigModule(), new InputSourceModule(), - new LookupSerdeModule(), - new NoopSegmentMetadataCacheModule() + new LookupSerdeModule() ); } diff --git a/services/src/main/java/org/apache/druid/cli/CliOverlord.java b/services/src/main/java/org/apache/druid/cli/CliOverlord.java index 71dabdde6df1..95fa654674d9 100644 --- a/services/src/main/java/org/apache/druid/cli/CliOverlord.java +++ b/services/src/main/java/org/apache/druid/cli/CliOverlord.java @@ -50,6 +50,7 @@ import org.apache.druid.guice.LifecycleModule; import org.apache.druid.guice.ListProvider; import org.apache.druid.guice.ManageLifecycle; +import org.apache.druid.guice.MetadataManagerModule; import org.apache.druid.guice.PolyBind; import org.apache.druid.guice.SupervisorModule; import org.apache.druid.guice.annotations.Json; @@ -108,21 +109,14 @@ import org.apache.druid.indexing.worker.shuffle.IntermediaryDataManager; import org.apache.druid.indexing.worker.shuffle.LocalIntermediaryDataManager; import org.apache.druid.java.util.common.logger.Logger; -import org.apache.druid.metadata.SegmentsMetadataManager; -import org.apache.druid.metadata.SegmentsMetadataManagerProvider; import org.apache.druid.metadata.input.InputSourceModule; -import org.apache.druid.metadata.segment.cache.HeapMemorySegmentMetadataCache; -import org.apache.druid.metadata.segment.cache.SegmentMetadataCache; import org.apache.druid.query.lookup.LookupSerdeModule; import org.apache.druid.segment.incremental.RowIngestionMetersFactory; -import org.apache.druid.segment.metadata.NoopSegmentSchemaCache; -import org.apache.druid.segment.metadata.SegmentSchemaCache; import org.apache.druid.segment.realtime.ChatHandlerProvider; import org.apache.druid.segment.realtime.NoopChatHandlerProvider; import org.apache.druid.segment.realtime.appenderator.AppenderatorsManager; import org.apache.druid.segment.realtime.appenderator.DummyForInjectionAppenderatorsManager; import org.apache.druid.server.compaction.CompactionStatusTracker; -import org.apache.druid.server.coordinator.CoordinatorConfigManager; import org.apache.druid.server.coordinator.CoordinatorOverlordServiceConfig; import org.apache.druid.server.coordinator.DruidCompactionConfig; import org.apache.druid.server.http.RedirectFilter; @@ -200,6 +194,7 @@ public void configure(Properties properties) protected List getModules(final boolean standalone) { return ImmutableList.of( + standalone ? new MetadataManagerModule() : binder -> {}, new Module() { @Override @@ -215,17 +210,6 @@ public void configure(Binder binder) binder.bindConstant().annotatedWith(Names.named("tlsServicePort")).to(8290); binder.bind(CompactionStatusTracker.class).in(LazySingleton.class); - binder.bind(SegmentsMetadataManager.class) - .toProvider(SegmentsMetadataManagerProvider.class) - .in(ManageLifecycle.class); - binder.bind(CoordinatorConfigManager.class).in(LazySingleton.class); - - binder.bind(SegmentMetadataCache.class) - .to(HeapMemorySegmentMetadataCache.class) - .in(LazySingleton.class); - binder.bind(SegmentSchemaCache.class) - .to(NoopSegmentSchemaCache.class) - .in(LazySingleton.class); } JsonConfigProvider.bind(binder, "druid.coordinator.asOverlord", CoordinatorOverlordServiceConfig.class); diff --git a/services/src/main/java/org/apache/druid/cli/CliPeon.java b/services/src/main/java/org/apache/druid/cli/CliPeon.java index 73a51f9896ae..26edb68c7ee4 100644 --- a/services/src/main/java/org/apache/druid/cli/CliPeon.java +++ b/services/src/main/java/org/apache/druid/cli/CliPeon.java @@ -57,7 +57,7 @@ import org.apache.druid.guice.LifecycleModule; import org.apache.druid.guice.ManageLifecycle; import org.apache.druid.guice.ManageLifecycleServer; -import org.apache.druid.guice.NoopSegmentMetadataCacheModule; +import org.apache.druid.guice.MetadataManagerModule; import org.apache.druid.guice.PeonProcessingModule; import org.apache.druid.guice.PolyBind; import org.apache.druid.guice.QueryRunnerFactoryModule; @@ -88,7 +88,6 @@ import org.apache.druid.indexing.common.task.batch.parallel.ParallelIndexSupervisorTaskClientProviderImpl; import org.apache.druid.indexing.common.task.batch.parallel.ShuffleClient; import org.apache.druid.indexing.overlord.HeapMemoryTaskStorage; -import org.apache.druid.indexing.overlord.IndexerMetadataStorageCoordinator; import org.apache.druid.indexing.overlord.SingleTaskBackgroundRunner; import org.apache.druid.indexing.overlord.TaskRunner; import org.apache.druid.indexing.overlord.TaskStorage; @@ -100,7 +99,6 @@ import org.apache.druid.indexing.worker.shuffle.LocalIntermediaryDataManager; import org.apache.druid.java.util.common.lifecycle.Lifecycle; import org.apache.druid.java.util.common.logger.Logger; -import org.apache.druid.metadata.IndexerSQLMetadataStorageCoordinator; import org.apache.druid.metadata.input.InputSourceModule; import org.apache.druid.query.DruidMetrics; import org.apache.druid.query.QuerySegmentWalker; @@ -222,6 +220,7 @@ public void configure(Properties properties) protected List getModules() { return ImmutableList.of( + new MetadataManagerModule(), // needed here only to support druid.peon.mode=local new PeonProcessingModule(), new QueryableModule(), new QueryRunnerFactoryModule(), @@ -384,8 +383,7 @@ public LocalTmpStorageConfig getLocalTmpStorage() new IndexingServiceTuningConfigModule(), new InputSourceModule(), new ChatHandlerServerModule(properties), - new LookupModule(), - new NoopSegmentMetadataCacheModule() + new LookupModule() ); } @@ -503,9 +501,6 @@ private static void configureTaskActionClient(Binder binder) JsonConfigProvider.bind(binder, "druid.indexer.storage", TaskStorageConfig.class); binder.bind(TaskStorage.class).to(HeapMemoryTaskStorage.class).in(LazySingleton.class); binder.bind(TaskActionToolbox.class).in(LazySingleton.class); - binder.bind(IndexerMetadataStorageCoordinator.class) - .to(IndexerSQLMetadataStorageCoordinator.class) - .in(LazySingleton.class); taskActionBinder .addBinding("remote") .to(RemoteTaskActionClientFactory.class) diff --git a/services/src/main/java/org/apache/druid/cli/CliRouter.java b/services/src/main/java/org/apache/druid/cli/CliRouter.java index 66e81e3e789a..a3344aa4e7fe 100644 --- a/services/src/main/java/org/apache/druid/cli/CliRouter.java +++ b/services/src/main/java/org/apache/druid/cli/CliRouter.java @@ -33,7 +33,6 @@ import org.apache.druid.guice.LazySingleton; import org.apache.druid.guice.LifecycleModule; import org.apache.druid.guice.ManageLifecycle; -import org.apache.druid.guice.NoopSegmentMetadataCacheModule; import org.apache.druid.guice.QueryRunnerFactoryModule; import org.apache.druid.guice.QueryableModule; import org.apache.druid.guice.RouterProcessingModule; @@ -133,8 +132,7 @@ protected List getModules() .toProvider(new LocalTmpStorageConfig.DefaultLocalTmpStorageConfigProvider("router")) .in(LazySingleton.class); }, - new LookupSerdeModule(), - new NoopSegmentMetadataCacheModule() + new LookupSerdeModule() ); } } diff --git a/services/src/main/java/org/apache/druid/guice/MetadataManagerModule.java b/services/src/main/java/org/apache/druid/guice/MetadataManagerModule.java new file mode 100644 index 000000000000..1cc168318cc2 --- /dev/null +++ b/services/src/main/java/org/apache/druid/guice/MetadataManagerModule.java @@ -0,0 +1,108 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.guice; + +import com.google.inject.Binder; +import com.google.inject.Inject; +import com.google.inject.Module; +import org.apache.druid.discovery.NodeRole; +import org.apache.druid.guice.annotations.Self; +import org.apache.druid.indexing.overlord.IndexerMetadataStorageCoordinator; +import org.apache.druid.metadata.IndexerSQLMetadataStorageCoordinator; +import org.apache.druid.metadata.MetadataRuleManager; +import org.apache.druid.metadata.MetadataRuleManagerProvider; +import org.apache.druid.metadata.MetadataSupervisorManager; +import org.apache.druid.metadata.SQLMetadataRuleManagerProvider; +import org.apache.druid.metadata.SQLMetadataSupervisorManager; +import org.apache.druid.metadata.SegmentsMetadataManager; +import org.apache.druid.metadata.SegmentsMetadataManagerProvider; +import org.apache.druid.metadata.SqlSegmentsMetadataManagerProvider; +import org.apache.druid.metadata.segment.SegmentMetadataTransactionFactory; +import org.apache.druid.metadata.segment.SqlSegmentMetadataTransactionFactory; +import org.apache.druid.metadata.segment.cache.HeapMemorySegmentMetadataCache; +import org.apache.druid.metadata.segment.cache.SegmentMetadataCache; +import org.apache.druid.segment.metadata.NoopSegmentSchemaCache; +import org.apache.druid.segment.metadata.SegmentSchemaCache; +import org.apache.druid.server.coordinator.CoordinatorConfigManager; +import org.apache.druid.server.coordinator.MetadataManager; + +import java.util.Set; + +/** + * Module used by Overlord and Coordinator to bind various metadata managers. + */ +public class MetadataManagerModule implements Module +{ + private Set nodeRoles; + + @Inject + public void configure( + @Self Set nodeRoles + ) + { + this.nodeRoles = nodeRoles; + } + + @Override + public void configure(Binder binder) + { + if (nodeRoles.contains(NodeRole.COORDINATOR)) { + binder.bind(MetadataRuleManagerProvider.class) + .to(SQLMetadataRuleManagerProvider.class) + .in(LazySingleton.class); + binder.bind(MetadataRuleManager.class) + .toProvider(MetadataRuleManagerProvider.class) + .in(ManageLifecycle.class); + + binder.bind(MetadataManager.class).in(LazySingleton.class); + } + + binder.bind(CoordinatorConfigManager.class).in(LazySingleton.class); + + binder.bind(MetadataSupervisorManager.class) + .to(SQLMetadataSupervisorManager.class) + .in(LazySingleton.class); + + binder.bind(SegmentsMetadataManagerProvider.class) + .to(SqlSegmentsMetadataManagerProvider.class) + .in(LazySingleton.class); + binder.bind(SegmentsMetadataManager.class) + .toProvider(SegmentsMetadataManagerProvider.class) + .in(ManageLifecycle.class); + + binder.bind(SegmentMetadataTransactionFactory.class) + .to(SqlSegmentMetadataTransactionFactory.class) + .in(LazySingleton.class); + binder.bind(IndexerMetadataStorageCoordinator.class) + .to(IndexerSQLMetadataStorageCoordinator.class) + .in(ManageLifecycle.class); + binder.bind(SegmentMetadataCache.class) + .to(HeapMemorySegmentMetadataCache.class) + .in(LazySingleton.class); + + if (nodeRoles.contains(NodeRole.COORDINATOR)) { + binder.bind(SegmentSchemaCache.class).in(LazySingleton.class); + } else { + binder.bind(SegmentSchemaCache.class) + .to(NoopSegmentSchemaCache.class) + .in(LazySingleton.class); + } + } +} diff --git a/services/src/main/java/org/apache/druid/guice/NoopSegmentMetadataCacheModule.java b/services/src/main/java/org/apache/druid/guice/NoopSegmentMetadataCacheModule.java deleted file mode 100644 index d9e1e2bf1548..000000000000 --- a/services/src/main/java/org/apache/druid/guice/NoopSegmentMetadataCacheModule.java +++ /dev/null @@ -1,47 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -package org.apache.druid.guice; - -import com.google.inject.Binder; -import com.google.inject.Module; -import org.apache.druid.metadata.segment.cache.NoopSegmentMetadataCache; -import org.apache.druid.metadata.segment.cache.SegmentMetadataCache; -import org.apache.druid.segment.metadata.NoopSegmentSchemaCache; -import org.apache.druid.segment.metadata.SegmentSchemaCache; - -/** - * Module used by services other than Overlord and Coordinator to bind - * {@link SegmentMetadataCache} and {@link SegmentSchemaCache} to noop instances. - *

- * Classes using these caches like {@code SqlSegmentMetadataTransactionFactory} - * and {@code SqlSegmentsMetadataManagerProvider} are currently bound for all - * services in {@code SQLMetadataStorageDruidModule}. Ideally, this module should - * be installed only for Coordinator/Overlord since other services do not access - * the metadata store directly. - */ -public class NoopSegmentMetadataCacheModule implements Module -{ - @Override - public void configure(Binder binder) - { - binder.bind(SegmentMetadataCache.class).to(NoopSegmentMetadataCache.class).in(LazySingleton.class); - binder.bind(SegmentSchemaCache.class).to(NoopSegmentSchemaCache.class).in(LazySingleton.class); - } -} diff --git a/services/src/main/java/org/apache/druid/guice/SegmentSchemaCacheModule.java b/services/src/main/java/org/apache/druid/guice/SegmentSchemaCacheModule.java index 859abc76c8b7..744d56a89e02 100644 --- a/services/src/main/java/org/apache/druid/guice/SegmentSchemaCacheModule.java +++ b/services/src/main/java/org/apache/druid/guice/SegmentSchemaCacheModule.java @@ -44,7 +44,6 @@ import org.apache.druid.segment.metadata.CentralizedDatasourceSchemaConfig; import org.apache.druid.segment.metadata.CoordinatorSegmentMetadataCache; import org.apache.druid.segment.metadata.SegmentMetadataQuerySegmentWalker; -import org.apache.druid.segment.metadata.SegmentSchemaCache; import org.apache.druid.server.QueryScheduler; import org.apache.druid.server.QuerySchedulerProvider; @@ -84,7 +83,6 @@ public void configure(Binder binder) .in(LazySingleton.class); binder.bind(QuerySchedulerProvider.class).in(LazySingleton.class); binder.bind(QuerySegmentWalker.class).to(SegmentMetadataQuerySegmentWalker.class).in(LazySingleton.class); - binder.bind(SegmentSchemaCache.class).in(LazySingleton.class); LifecycleModule.register(binder, CoordinatorSegmentMetadataCache.class); } From 6ce338b6f1cdc7bfdcbba1ac9c670071e7245154 Mon Sep 17 00:00:00 2001 From: Kashif Faraz Date: Sat, 17 May 2025 02:20:40 +0530 Subject: [PATCH 10/18] Simplify some dependencies --- .../MaterializedViewSupervisorTest.java | 2 - .../DatasourceOptimizerTest.java | 2 - .../common/actions/TaskActionTestKit.java | 5 - .../common/task/IngestionTestBase.java | 3 - .../overlord/TaskLockBoxConcurrencyTest.java | 3 - .../indexing/overlord/TaskLockboxTest.java | 3 - .../indexing/overlord/TaskQueueScaleTest.java | 3 - .../SeekableStreamIndexTaskTestBase.java | 2 - .../druid/guice/MetadataConfigModule.java | 28 +++-- .../SegmentMetadataTransactionFactory.java | 25 ---- ...entMetadataReadOnlyTransactionFactory.java | 114 ++++++++++++++++++ .../SqlSegmentMetadataTransactionFactory.java | 62 +--------- .../cache/HeapMemorySegmentMetadataCache.java | 3 +- .../segment/metadata/SegmentSchemaCache.java | 2 +- ...etadataStorageCoordinatorMarkUsedTest.java | 2 - ...etadataStorageCoordinatorReadOnlyTest.java | 28 +++-- ...exerSQLMetadataStorageCoordinatorTest.java | 3 - ...orageCoordinatorSchemaPersistenceTest.java | 2 - .../SqlSegmentsMetadataManagerV2Test.java | 4 +- .../HeapMemorySegmentMetadataCacheTest.java | 2 - .../org/apache/druid/cli/CliCoordinator.java | 3 +- .../java/org/apache/druid/cli/CliPeon.java | 30 +---- .../org/apache/druid/cli/ServerRunnable.java | 14 +-- .../druid/guice/MetadataManagerModule.java | 59 ++++++--- 24 files changed, 214 insertions(+), 190 deletions(-) create mode 100644 server/src/main/java/org/apache/druid/metadata/segment/SqlSegmentMetadataReadOnlyTransactionFactory.java diff --git a/extensions-contrib/materialized-view-maintenance/src/test/java/org/apache/druid/indexing/materializedview/MaterializedViewSupervisorTest.java b/extensions-contrib/materialized-view-maintenance/src/test/java/org/apache/druid/indexing/materializedview/MaterializedViewSupervisorTest.java index 62b3f4af3d7f..01964ce28260 100644 --- a/extensions-contrib/materialized-view-maintenance/src/test/java/org/apache/druid/indexing/materializedview/MaterializedViewSupervisorTest.java +++ b/extensions-contrib/materialized-view-maintenance/src/test/java/org/apache/druid/indexing/materializedview/MaterializedViewSupervisorTest.java @@ -28,7 +28,6 @@ import junit.framework.AssertionFailedError; import org.apache.druid.data.input.impl.DimensionsSpec; import org.apache.druid.data.input.impl.StringDimensionSchema; -import org.apache.druid.discovery.NodeRole; import org.apache.druid.error.EntryAlreadyExists; import org.apache.druid.indexer.HadoopIOConfig; import org.apache.druid.indexer.HadoopIngestionSpec; @@ -113,7 +112,6 @@ public void setUp() derbyConnectorRule.metadataTablesConfigSupplier().get(), derbyConnector, new TestDruidLeaderSelector(), - Set.of(NodeRole.OVERLORD), NoopSegmentMetadataCache.instance(), NoopServiceEmitter.instance() ), diff --git a/extensions-contrib/materialized-view-selection/src/test/java/org/apache/druid/query/materializedview/DatasourceOptimizerTest.java b/extensions-contrib/materialized-view-selection/src/test/java/org/apache/druid/query/materializedview/DatasourceOptimizerTest.java index 755b0efddace..1ec98359db79 100644 --- a/extensions-contrib/materialized-view-selection/src/test/java/org/apache/druid/query/materializedview/DatasourceOptimizerTest.java +++ b/extensions-contrib/materialized-view-selection/src/test/java/org/apache/druid/query/materializedview/DatasourceOptimizerTest.java @@ -37,7 +37,6 @@ import org.apache.druid.client.selector.HighestPriorityTierSelectorStrategy; import org.apache.druid.client.selector.RandomServerSelectorStrategy; import org.apache.druid.curator.CuratorTestBase; -import org.apache.druid.discovery.NodeRole; import org.apache.druid.indexing.materializedview.DerivativeDataSourceMetadata; import org.apache.druid.jackson.DefaultObjectMapper; import org.apache.druid.java.util.common.Intervals; @@ -123,7 +122,6 @@ public void setUp() throws Exception derbyConnectorRule.metadataTablesConfigSupplier().get(), derbyConnector, new TestDruidLeaderSelector(), - Set.of(NodeRole.OVERLORD), NoopSegmentMetadataCache.instance(), NoopServiceEmitter.instance() ), diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/common/actions/TaskActionTestKit.java b/indexing-service/src/test/java/org/apache/druid/indexing/common/actions/TaskActionTestKit.java index a5fb2cb04a84..8efcfa58b1e8 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/common/actions/TaskActionTestKit.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/common/actions/TaskActionTestKit.java @@ -21,7 +21,6 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.base.Suppliers; -import org.apache.druid.discovery.NodeRole; import org.apache.druid.indexing.common.TestUtils; import org.apache.druid.indexing.common.config.TaskStorageConfig; import org.apache.druid.indexing.overlord.HeapMemoryTaskStorage; @@ -50,8 +49,6 @@ import org.joda.time.Period; import org.junit.rules.ExternalResource; -import java.util.Set; - public class TaskActionTestKit extends ExternalResource { private final MetadataStorageTablesConfig metadataStorageTablesConfig = MetadataStorageTablesConfig.fromBase("druid"); @@ -178,7 +175,6 @@ private SqlSegmentMetadataTransactionFactory setupTransactionFactory(ObjectMappe objectMapper, Suppliers.ofInstance(new SegmentsMetadataManagerConfig(Period.seconds(1), cacheMode)), Suppliers.ofInstance(metadataStorageTablesConfig), - Suppliers.ofInstance(CentralizedDatasourceSchemaConfig.create()), new SegmentSchemaCache(), testDerbyConnector, (poolSize, name) -> new WrappingScheduledExecutorService(name, metadataCachePollExec, false), @@ -193,7 +189,6 @@ private SqlSegmentMetadataTransactionFactory setupTransactionFactory(ObjectMappe metadataStorageTablesConfig, testDerbyConnector, leaderSelector, - Set.of(NodeRole.OVERLORD), segmentMetadataCache, NoopServiceEmitter.instance() ) diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IngestionTestBase.java b/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IngestionTestBase.java index 5f6402ce299b..a56d7acc5097 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IngestionTestBase.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IngestionTestBase.java @@ -34,7 +34,6 @@ import org.apache.druid.data.input.impl.ParseSpec; import org.apache.druid.data.input.impl.RegexInputFormat; import org.apache.druid.data.input.impl.RegexParseSpec; -import org.apache.druid.discovery.NodeRole; import org.apache.druid.indexer.TaskStatus; import org.apache.druid.indexer.report.IngestionStatsAndErrors; import org.apache.druid.indexer.report.IngestionStatsAndErrorsTaskReport; @@ -325,7 +324,6 @@ private SqlSegmentMetadataTransactionFactory createTransactionFactory() objectMapper, Suppliers.ofInstance(new SegmentsMetadataManagerConfig(Period.millis(10), cacheMode)), derbyConnectorRule.metadataTablesConfigSupplier(), - Suppliers.ofInstance(CentralizedDatasourceSchemaConfig.create()), new SegmentSchemaCache(), derbyConnectorRule.getConnector(), ScheduledExecutors::fixed, @@ -340,7 +338,6 @@ private SqlSegmentMetadataTransactionFactory createTransactionFactory() derbyConnectorRule.metadataTablesConfigSupplier().get(), derbyConnectorRule.getConnector(), leaderSelector, - Set.of(NodeRole.OVERLORD), segmentMetadataCache, NoopServiceEmitter.instance() ); diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/TaskLockBoxConcurrencyTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/TaskLockBoxConcurrencyTest.java index a6eb19f47830..a55117aad12d 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/TaskLockBoxConcurrencyTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/TaskLockBoxConcurrencyTest.java @@ -22,7 +22,6 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.collect.ImmutableList; import org.apache.druid.common.guava.SettableSupplier; -import org.apache.druid.discovery.NodeRole; import org.apache.druid.indexer.TaskStatus; import org.apache.druid.indexing.common.TaskLockType; import org.apache.druid.indexing.common.config.TaskStorageConfig; @@ -50,7 +49,6 @@ import java.util.Collections; import java.util.HashSet; import java.util.List; -import java.util.Set; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; @@ -91,7 +89,6 @@ public void setup() derby.metadataTablesConfigSupplier().get(), derbyConnector, new TestDruidLeaderSelector(), - Set.of(NodeRole.OVERLORD), NoopSegmentMetadataCache.instance(), NoopServiceEmitter.instance() ), diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/TaskLockboxTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/TaskLockboxTest.java index 8c2c78611acb..48c5b5c4e94a 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/TaskLockboxTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/TaskLockboxTest.java @@ -29,7 +29,6 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; -import org.apache.druid.discovery.NodeRole; import org.apache.druid.indexer.TaskStatus; import org.apache.druid.indexing.common.LockGranularity; import org.apache.druid.indexing.common.SegmentLock; @@ -138,7 +137,6 @@ public void setup() tablesConfig, derbyConnector, new TestDruidLeaderSelector(), - Set.of(NodeRole.OVERLORD), NoopSegmentMetadataCache.instance(), NoopServiceEmitter.instance() ), @@ -481,7 +479,6 @@ public void testSyncWithUnknownTaskTypesFromModuleNotLoaded() derby.metadataTablesConfigSupplier().get(), derbyConnector, new TestDruidLeaderSelector(), - Set.of(NodeRole.OVERLORD), NoopSegmentMetadataCache.instance(), NoopServiceEmitter.instance() ), diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/TaskQueueScaleTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/TaskQueueScaleTest.java index 1900752e8be2..f54ea7fe5860 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/TaskQueueScaleTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/TaskQueueScaleTest.java @@ -25,7 +25,6 @@ import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.SettableFuture; import com.google.errorprone.annotations.concurrent.GuardedBy; -import org.apache.druid.discovery.NodeRole; import org.apache.druid.indexer.RunnerTaskState; import org.apache.druid.indexer.TaskLocation; import org.apache.druid.indexer.TaskStatus; @@ -70,7 +69,6 @@ import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.Set; import java.util.concurrent.Executor; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; @@ -113,7 +111,6 @@ public void setUp() derbyConnectorRule.metadataTablesConfigSupplier().get(), derbyConnectorRule.getConnector(), new TestDruidLeaderSelector(), - Set.of(NodeRole.OVERLORD), NoopSegmentMetadataCache.instance(), NoopServiceEmitter.instance() ), diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/SeekableStreamIndexTaskTestBase.java b/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/SeekableStreamIndexTaskTestBase.java index 485b6699d1b8..e0332708c3fe 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/SeekableStreamIndexTaskTestBase.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/SeekableStreamIndexTaskTestBase.java @@ -47,7 +47,6 @@ import org.apache.druid.discovery.DataNodeService; import org.apache.druid.discovery.DruidNodeAnnouncer; import org.apache.druid.discovery.LookupNodeService; -import org.apache.druid.discovery.NodeRole; import org.apache.druid.error.DruidException; import org.apache.druid.indexer.TaskStatus; import org.apache.druid.indexer.granularity.UniformGranularitySpec; @@ -599,7 +598,6 @@ protected void makeToolboxFactory(TestUtils testUtils, ServiceEmitter emitter, b derby.metadataTablesConfigSupplier().get(), derbyConnector, new TestDruidLeaderSelector(), - Set.of(NodeRole.OVERLORD), NoopSegmentMetadataCache.instance(), NoopServiceEmitter.instance() ), diff --git a/server/src/main/java/org/apache/druid/guice/MetadataConfigModule.java b/server/src/main/java/org/apache/druid/guice/MetadataConfigModule.java index 1ca4578e5603..7a429049db86 100644 --- a/server/src/main/java/org/apache/druid/guice/MetadataConfigModule.java +++ b/server/src/main/java/org/apache/druid/guice/MetadataConfigModule.java @@ -21,29 +21,43 @@ import com.google.inject.Binder; import com.google.inject.Module; -import org.apache.druid.metadata.MetadataRuleManagerConfig; import org.apache.druid.metadata.MetadataStorageConnectorConfig; import org.apache.druid.metadata.MetadataStorageTablesConfig; -import org.apache.druid.metadata.SegmentsMetadataManagerConfig; import org.apache.druid.segment.metadata.CentralizedDatasourceSchemaConfig; +import java.util.Properties; + +/** + * Binds the following metadata configs for all services: + *

    + *
  • {@link MetadataStorageTablesConfig}
    • + *
  • {@link MetadataStorageConnectorConfig}
    • + *
  • {@link CentralizedDatasourceSchemaConfig}
    • + *
+ * Ideally, the storage configs should be bound only on Coordinator and Overlord, + * but they are needed for other services too since metadata storage extensions + * are currently loaded on all services. + */ public class MetadataConfigModule implements Module { + public static final String CENTRALIZED_DATASOURCE_SCHEMA_ENABLED = + CentralizedDatasourceSchemaConfig.PROPERTY_PREFIX + ".enabled"; + @Override public void configure(Binder binder) { JsonConfigProvider.bind(binder, MetadataStorageTablesConfig.PROPERTY_BASE, MetadataStorageTablesConfig.class); JsonConfigProvider.bind(binder, MetadataStorageConnectorConfig.PROPERTY_BASE, MetadataStorageConnectorConfig.class); - JsonConfigProvider.bind(binder, "druid.manager.segments", SegmentsMetadataManagerConfig.class); - JsonConfigProvider.bind(binder, "druid.manager.rules", MetadataRuleManagerConfig.class); - - // SegmentSchemaCacheConfig needs to be bound on all services since - // it is a dependency of SqlSegmentsMetadataManager (both legacy and V2) JsonConfigProvider.bind( binder, CentralizedDatasourceSchemaConfig.PROPERTY_PREFIX, CentralizedDatasourceSchemaConfig.class ); } + + public static boolean isSegmentSchemaCacheEnabled(Properties properties) + { + return Boolean.parseBoolean(properties.getProperty(CENTRALIZED_DATASOURCE_SCHEMA_ENABLED)); + } } diff --git a/server/src/main/java/org/apache/druid/metadata/segment/SegmentMetadataTransactionFactory.java b/server/src/main/java/org/apache/druid/metadata/segment/SegmentMetadataTransactionFactory.java index 6bfbf564c964..e048e10970c6 100644 --- a/server/src/main/java/org/apache/druid/metadata/segment/SegmentMetadataTransactionFactory.java +++ b/server/src/main/java/org/apache/druid/metadata/segment/SegmentMetadataTransactionFactory.java @@ -41,29 +41,4 @@ T inReadWriteDatasourceTransaction( String dataSource, SegmentMetadataTransaction.Callback callback ); - - /** - * No-op instance of {@link SegmentMetadataTransactionFactory} which does not - * support any operation. - */ - SegmentMetadataTransactionFactory NOOP = new SegmentMetadataTransactionFactory() - { - @Override - public T inReadOnlyDatasourceTransaction( - String dataSource, - SegmentMetadataReadTransaction.Callback callback - ) - { - throw new UnsupportedOperationException(); - } - - @Override - public T inReadWriteDatasourceTransaction( - String dataSource, - SegmentMetadataTransaction.Callback callback - ) - { - throw new UnsupportedOperationException(); - } - }; } diff --git a/server/src/main/java/org/apache/druid/metadata/segment/SqlSegmentMetadataReadOnlyTransactionFactory.java b/server/src/main/java/org/apache/druid/metadata/segment/SqlSegmentMetadataReadOnlyTransactionFactory.java new file mode 100644 index 000000000000..0d0d1e42a1e2 --- /dev/null +++ b/server/src/main/java/org/apache/druid/metadata/segment/SqlSegmentMetadataReadOnlyTransactionFactory.java @@ -0,0 +1,114 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.metadata.segment; + +import com.fasterxml.jackson.databind.ObjectMapper; +import com.google.inject.Inject; +import org.apache.druid.error.DruidException; +import org.apache.druid.metadata.MetadataStorageTablesConfig; +import org.apache.druid.metadata.SQLMetadataConnector; +import org.skife.jdbi.v2.Handle; +import org.skife.jdbi.v2.TransactionStatus; + +/** + * Factory for read-only {@link SegmentMetadataTransaction}s that always read + * directly from the metadata store and never from the {@code SegmentMetadataCache}. + * + * @see SqlSegmentMetadataTransactionFactory + */ +public class SqlSegmentMetadataReadOnlyTransactionFactory implements SegmentMetadataTransactionFactory +{ + private static final int QUIET_RETRIES = 2; + private static final int MAX_RETRIES = 3; + + private final ObjectMapper jsonMapper; + private final MetadataStorageTablesConfig tablesConfig; + private final SQLMetadataConnector connector; + + @Inject + public SqlSegmentMetadataReadOnlyTransactionFactory( + ObjectMapper jsonMapper, + MetadataStorageTablesConfig tablesConfig, + SQLMetadataConnector connector + ) + { + this.jsonMapper = jsonMapper; + this.tablesConfig = tablesConfig; + this.connector = connector; + } + + public int getMaxRetries() + { + return MAX_RETRIES; + } + + public int getQuietRetries() + { + return QUIET_RETRIES; + } + + @Override + public T inReadOnlyDatasourceTransaction( + String dataSource, + SegmentMetadataReadTransaction.Callback callback + ) + { + return connector.retryReadOnlyTransaction( + (handle, status) -> { + final SegmentMetadataTransaction sqlTransaction + = createSqlTransaction(dataSource, handle, status); + return executeReadAndClose(sqlTransaction, callback); + }, + QUIET_RETRIES, + getMaxRetries() + ); + } + + @Override + public T inReadWriteDatasourceTransaction( + String dataSource, + SegmentMetadataTransaction.Callback callback + ) + { + throw DruidException.defensive("Only Overlord can perform write transactions on segment metadata."); + } + + protected SegmentMetadataTransaction createSqlTransaction( + String dataSource, + Handle handle, + TransactionStatus transactionStatus + ) + { + return new SqlSegmentMetadataTransaction( + dataSource, + handle, transactionStatus, connector, tablesConfig, jsonMapper + ); + } + + protected T executeReadAndClose( + SegmentMetadataReadTransaction transaction, + SegmentMetadataReadTransaction.Callback callback + ) throws Exception + { + try (transaction) { + return callback.inTransaction(transaction); + } + } +} diff --git a/server/src/main/java/org/apache/druid/metadata/segment/SqlSegmentMetadataTransactionFactory.java b/server/src/main/java/org/apache/druid/metadata/segment/SqlSegmentMetadataTransactionFactory.java index 2942ed1a0861..26083e44b678 100644 --- a/server/src/main/java/org/apache/druid/metadata/segment/SqlSegmentMetadataTransactionFactory.java +++ b/server/src/main/java/org/apache/druid/metadata/segment/SqlSegmentMetadataTransactionFactory.java @@ -23,9 +23,6 @@ import com.google.inject.Inject; import org.apache.druid.client.indexing.IndexingService; import org.apache.druid.discovery.DruidLeaderSelector; -import org.apache.druid.discovery.NodeRole; -import org.apache.druid.error.DruidException; -import org.apache.druid.guice.annotations.Self; import org.apache.druid.java.util.common.logger.Logger; import org.apache.druid.java.util.emitter.service.ServiceEmitter; import org.apache.druid.java.util.emitter.service.ServiceMetricEvent; @@ -34,10 +31,6 @@ import org.apache.druid.metadata.segment.cache.Metric; import org.apache.druid.metadata.segment.cache.SegmentMetadataCache; import org.apache.druid.query.DruidMetrics; -import org.skife.jdbi.v2.Handle; -import org.skife.jdbi.v2.TransactionStatus; - -import java.util.Set; /** * Factory for {@link SegmentMetadataTransaction}s. If the @@ -56,46 +49,30 @@ * now, it continues to read directly from the metadata store for consistency * with older Druid versions. */ -public class SqlSegmentMetadataTransactionFactory implements SegmentMetadataTransactionFactory +public class SqlSegmentMetadataTransactionFactory extends SqlSegmentMetadataReadOnlyTransactionFactory { private static final Logger log = new Logger(SqlSegmentMetadataTransactionFactory.class); - private static final int QUIET_RETRIES = 2; - private static final int MAX_RETRIES = 3; - - private final ObjectMapper jsonMapper; - private final MetadataStorageTablesConfig tablesConfig; private final SQLMetadataConnector connector; private final DruidLeaderSelector leaderSelector; private final SegmentMetadataCache segmentMetadataCache; private final ServiceEmitter emitter; - private final boolean isNotOverlord; - @Inject public SqlSegmentMetadataTransactionFactory( ObjectMapper jsonMapper, MetadataStorageTablesConfig tablesConfig, SQLMetadataConnector connector, @IndexingService DruidLeaderSelector leaderSelector, - @Self Set nodeRoles, SegmentMetadataCache segmentMetadataCache, ServiceEmitter emitter ) { - this.jsonMapper = jsonMapper; - this.tablesConfig = tablesConfig; + super(jsonMapper, tablesConfig, connector); this.connector = connector; this.leaderSelector = leaderSelector; this.segmentMetadataCache = segmentMetadataCache; this.emitter = emitter; - - this.isNotOverlord = !nodeRoles.contains(NodeRole.OVERLORD); - } - - public int getMaxRetries() - { - return MAX_RETRIES; } @Override @@ -109,10 +86,7 @@ public T inReadOnlyDatasourceTransaction( final SegmentMetadataTransaction sqlTransaction = createSqlTransaction(dataSource, handle, status); - if (isNotOverlord) { - // Read directly from the metadata store if not Overlord - return executeReadAndClose(sqlTransaction, callback); - } else if (segmentMetadataCache.isSyncedForRead()) { + if (segmentMetadataCache.isSyncedForRead()) { // Use cache as it is already synced with the metadata store emitTransactionCount(Metric.READ_ONLY_TRANSACTIONS, dataSource); return segmentMetadataCache.readCacheForDataSource(dataSource, dataSourceCache -> { @@ -124,7 +98,7 @@ public T inReadOnlyDatasourceTransaction( return executeReadAndClose(sqlTransaction, callback); } }, - QUIET_RETRIES, + getQuietRetries(), getMaxRetries() ); } @@ -135,10 +109,6 @@ public T inReadWriteDatasourceTransaction( SegmentMetadataTransaction.Callback callback ) { - if (isNotOverlord) { - throw DruidException.defensive("Only Overlord can perform write transactions on segment metadata."); - } - return connector.retryTransaction( (handle, status) -> { final SegmentMetadataTransaction sqlTransaction @@ -167,23 +137,11 @@ public T inReadWriteDatasourceTransaction( return executeWriteAndClose(sqlTransaction, callback); } }, - QUIET_RETRIES, + getQuietRetries(), getMaxRetries() ); } - private SegmentMetadataTransaction createSqlTransaction( - String dataSource, - Handle handle, - TransactionStatus transactionStatus - ) - { - return new SqlSegmentMetadataTransaction( - dataSource, - handle, transactionStatus, connector, tablesConfig, jsonMapper - ); - } - private T executeWriteAndClose( SegmentMetadataTransaction transaction, SegmentMetadataTransaction.Callback callback @@ -201,16 +159,6 @@ private T executeWriteAndClose( } } - private T executeReadAndClose( - SegmentMetadataReadTransaction transaction, - SegmentMetadataReadTransaction.Callback callback - ) throws Exception - { - try (transaction) { - return callback.inTransaction(transaction); - } - } - private void emitTransactionCount(String metricName, String datasource) { emitter.emit( diff --git a/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java b/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java index 03a10b93fac3..7cb3b3780273 100644 --- a/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java +++ b/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java @@ -162,7 +162,6 @@ public HeapMemorySegmentMetadataCache( ObjectMapper jsonMapper, Supplier config, Supplier tablesConfig, - Supplier schemaConfig, SegmentSchemaCache segmentSchemaCache, SQLMetadataConnector connector, ScheduledExecutorFactory executorFactory, @@ -173,7 +172,7 @@ public HeapMemorySegmentMetadataCache( this.cacheMode = config.get().getCacheUsageMode(); this.pollDuration = config.get().getPollDuration().toStandardDuration(); this.tablesConfig = tablesConfig.get(); - this.useSchemaCache = schemaConfig.get().isEnabled() && segmentSchemaCache.isEnabled(); + this.useSchemaCache = segmentSchemaCache.isEnabled(); this.segmentSchemaCache = segmentSchemaCache; this.connector = connector; this.pollExecutor = isEnabled() diff --git a/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaCache.java b/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaCache.java index 2d46e601afd5..237b3944a176 100644 --- a/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaCache.java +++ b/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaCache.java @@ -95,7 +95,7 @@ public class SegmentSchemaCache private final AtomicInteger cacheMissCount = new AtomicInteger(0); /** - * @return true if this cache is enabled. + * @return true if schema caching is enabled. */ public boolean isEnabled() { diff --git a/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorMarkUsedTest.java b/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorMarkUsedTest.java index 7007607a2b0a..2406af6f12b6 100644 --- a/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorMarkUsedTest.java +++ b/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorMarkUsedTest.java @@ -20,7 +20,6 @@ package org.apache.druid.metadata; import com.google.common.collect.ImmutableList; -import org.apache.druid.discovery.NodeRole; import org.apache.druid.error.DruidException; import org.apache.druid.error.DruidExceptionMatcher; import org.apache.druid.indexing.overlord.IndexerMetadataStorageCoordinator; @@ -79,7 +78,6 @@ public void setup() derbyConnectorRule.metadataTablesConfigSupplier().get(), derbyConnector, new TestDruidLeaderSelector(), - Set.of(NodeRole.OVERLORD), NoopSegmentMetadataCache.instance(), NoopServiceEmitter.instance() ) diff --git a/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorReadOnlyTest.java b/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorReadOnlyTest.java index ada0ed1c887b..b8b72ff72e68 100644 --- a/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorReadOnlyTest.java +++ b/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorReadOnlyTest.java @@ -29,6 +29,7 @@ import org.apache.druid.java.util.common.Intervals; import org.apache.druid.java.util.metrics.StubServiceEmitter; import org.apache.druid.metadata.segment.SegmentMetadataTransactionFactory; +import org.apache.druid.metadata.segment.SqlSegmentMetadataReadOnlyTransactionFactory; import org.apache.druid.metadata.segment.SqlSegmentMetadataTransactionFactory; import org.apache.druid.metadata.segment.cache.HeapMemorySegmentMetadataCache; import org.apache.druid.metadata.segment.cache.SegmentMetadataCache; @@ -102,7 +103,6 @@ public void setup() mapper, () -> new SegmentsMetadataManagerConfig(null, cacheMode), derbyConnectorRule.metadataTablesConfigSupplier(), - () -> CentralizedDatasourceSchemaConfig.enabled(false), new SegmentSchemaCache(), derbyConnector, (corePoolSize, nameFormat) -> new WrappingScheduledExecutorService( @@ -154,15 +154,23 @@ private IndexerSQLMetadataStorageCoordinator createStorageCoordinator( NodeRole nodeRole ) { - final SegmentMetadataTransactionFactory transactionFactory = new SqlSegmentMetadataTransactionFactory( - mapper, - derbyConnectorRule.metadataTablesConfigSupplier().get(), - derbyConnector, - leaderSelector, - Set.of(nodeRole), - segmentMetadataCache, - emitter - ); + final SegmentMetadataTransactionFactory transactionFactory; + if (nodeRole.equals(NodeRole.COORDINATOR)) { + transactionFactory = new SqlSegmentMetadataReadOnlyTransactionFactory( + mapper, + derbyConnectorRule.metadataTablesConfigSupplier().get(), + derbyConnector + ); + } else { + transactionFactory = new SqlSegmentMetadataTransactionFactory( + mapper, + derbyConnectorRule.metadataTablesConfigSupplier().get(), + derbyConnector, + leaderSelector, + segmentMetadataCache, + emitter + ); + } return new IndexerSQLMetadataStorageCoordinator( transactionFactory, diff --git a/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java b/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java index 96d27bdda325..2562b3733510 100644 --- a/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java +++ b/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java @@ -24,7 +24,6 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import org.apache.druid.data.input.StringTuple; -import org.apache.druid.discovery.NodeRole; import org.apache.druid.error.ExceptionMatcher; import org.apache.druid.indexing.overlord.DataSourceMetadata; import org.apache.druid.indexing.overlord.ObjectMetadata; @@ -154,7 +153,6 @@ public void setUp() mapper, () -> new SegmentsMetadataManagerConfig(null, cacheMode), derbyConnectorRule.metadataTablesConfigSupplier(), - CentralizedDatasourceSchemaConfig::create, new SegmentSchemaCache(), derbyConnector, (corePoolSize, nameFormat) -> new WrappingScheduledExecutorService( @@ -180,7 +178,6 @@ public void setUp() derbyConnectorRule.metadataTablesConfigSupplier().get(), derbyConnector, leaderSelector, - Set.of(NodeRole.OVERLORD), segmentMetadataCache, emitter ) diff --git a/server/src/test/java/org/apache/druid/metadata/IndexerSqlMetadataStorageCoordinatorSchemaPersistenceTest.java b/server/src/test/java/org/apache/druid/metadata/IndexerSqlMetadataStorageCoordinatorSchemaPersistenceTest.java index 6fd536266d04..d98e54c3ea51 100644 --- a/server/src/test/java/org/apache/druid/metadata/IndexerSqlMetadataStorageCoordinatorSchemaPersistenceTest.java +++ b/server/src/test/java/org/apache/druid/metadata/IndexerSqlMetadataStorageCoordinatorSchemaPersistenceTest.java @@ -24,7 +24,6 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; -import org.apache.druid.discovery.NodeRole; import org.apache.druid.indexing.overlord.DataSourceMetadata; import org.apache.druid.indexing.overlord.SegmentPublishResult; import org.apache.druid.java.util.common.DateTimes; @@ -100,7 +99,6 @@ public void setUp() derbyConnectorRule.metadataTablesConfigSupplier().get(), derbyConnector, new TestDruidLeaderSelector(), - Set.of(NodeRole.OVERLORD), NoopSegmentMetadataCache.instance(), NoopServiceEmitter.instance() ); diff --git a/server/src/test/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2Test.java b/server/src/test/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2Test.java index 6b02e63498f1..e5554b931eef 100644 --- a/server/src/test/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2Test.java +++ b/server/src/test/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2Test.java @@ -33,6 +33,7 @@ import org.apache.druid.metadata.segment.cache.SegmentMetadataCache; import org.apache.druid.segment.TestDataSource; import org.apache.druid.segment.metadata.CentralizedDatasourceSchemaConfig; +import org.apache.druid.segment.metadata.NoopSegmentSchemaCache; import org.apache.druid.segment.metadata.SegmentSchemaCache; import org.apache.druid.server.coordinator.CreateDataSegments; import org.apache.druid.server.coordinator.simulate.BlockingExecutorService; @@ -89,8 +90,7 @@ private void initManager( jsonMapper, Suppliers.ofInstance(new SegmentsMetadataManagerConfig(Period.seconds(1), cacheMode)), Suppliers.ofInstance(storageConfig), - Suppliers.ofInstance(CentralizedDatasourceSchemaConfig.enabled(useSchemaCache)), - new SegmentSchemaCache(), + useSchemaCache ? new SegmentSchemaCache() : new NoopSegmentSchemaCache(), connector, (poolSize, name) -> new WrappingScheduledExecutorService(name, segmentMetadataCacheExec, false), emitter diff --git a/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java b/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java index f1a5bea26338..bdd02ba12d8d 100644 --- a/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java +++ b/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java @@ -34,7 +34,6 @@ import org.apache.druid.metadata.TestDerbyConnector; import org.apache.druid.segment.TestDataSource; import org.apache.druid.segment.TestHelper; -import org.apache.druid.segment.metadata.CentralizedDatasourceSchemaConfig; import org.apache.druid.segment.metadata.SegmentSchemaCache; import org.apache.druid.segment.realtime.appenderator.SegmentIdWithShardSpec; import org.apache.druid.server.coordinator.CreateDataSegments; @@ -106,7 +105,6 @@ private void setupTargetWithCaching(SegmentMetadataCache.UsageMode cacheMode) TestHelper.JSON_MAPPER, () -> metadataManagerConfig, derbyConnectorRule.metadataTablesConfigSupplier(), - CentralizedDatasourceSchemaConfig::create, new SegmentSchemaCache(), derbyConnector, executorFactory, diff --git a/services/src/main/java/org/apache/druid/cli/CliCoordinator.java b/services/src/main/java/org/apache/druid/cli/CliCoordinator.java index ffe06dda70a4..5a94d73a3e8b 100644 --- a/services/src/main/java/org/apache/druid/cli/CliCoordinator.java +++ b/services/src/main/java/org/apache/druid/cli/CliCoordinator.java @@ -47,6 +47,7 @@ import org.apache.druid.guice.LazySingleton; import org.apache.druid.guice.LifecycleModule; import org.apache.druid.guice.ManageLifecycle; +import org.apache.druid.guice.MetadataConfigModule; import org.apache.druid.guice.MetadataManagerModule; import org.apache.druid.guice.QueryableModule; import org.apache.druid.guice.SegmentSchemaCacheModule; @@ -141,7 +142,7 @@ public void configure(Properties properties) { this.properties = properties; beOverlord = isOverlord(properties); - isSegmentSchemaCacheEnabled = isSegmentSchemaCacheEnabled(properties); + isSegmentSchemaCacheEnabled = MetadataConfigModule.isSegmentSchemaCacheEnabled(properties); if (beOverlord) { log.info("Coordinator is configured to act as Overlord as well (%s = true).", AS_OVERLORD_PROPERTY); diff --git a/services/src/main/java/org/apache/druid/cli/CliPeon.java b/services/src/main/java/org/apache/druid/cli/CliPeon.java index 26edb68c7ee4..e9cbab108d39 100644 --- a/services/src/main/java/org/apache/druid/cli/CliPeon.java +++ b/services/src/main/java/org/apache/druid/cli/CliPeon.java @@ -57,7 +57,6 @@ import org.apache.druid.guice.LifecycleModule; import org.apache.druid.guice.ManageLifecycle; import org.apache.druid.guice.ManageLifecycleServer; -import org.apache.druid.guice.MetadataManagerModule; import org.apache.druid.guice.PeonProcessingModule; import org.apache.druid.guice.PolyBind; import org.apache.druid.guice.QueryRunnerFactoryModule; @@ -74,12 +73,9 @@ import org.apache.druid.indexing.common.RetryPolicyConfig; import org.apache.druid.indexing.common.RetryPolicyFactory; import org.apache.druid.indexing.common.TaskToolboxFactory; -import org.apache.druid.indexing.common.actions.LocalTaskActionClientFactory; import org.apache.druid.indexing.common.actions.RemoteTaskActionClientFactory; import org.apache.druid.indexing.common.actions.TaskActionClientFactory; -import org.apache.druid.indexing.common.actions.TaskActionToolbox; import org.apache.druid.indexing.common.config.TaskConfig; -import org.apache.druid.indexing.common.config.TaskStorageConfig; import org.apache.druid.indexing.common.stats.DropwizardRowIngestionMetersFactory; import org.apache.druid.indexing.common.task.Task; import org.apache.druid.indexing.common.task.batch.parallel.DeepStorageShuffleClient; @@ -87,10 +83,8 @@ import org.apache.druid.indexing.common.task.batch.parallel.ParallelIndexSupervisorTaskClientProvider; import org.apache.druid.indexing.common.task.batch.parallel.ParallelIndexSupervisorTaskClientProviderImpl; import org.apache.druid.indexing.common.task.batch.parallel.ShuffleClient; -import org.apache.druid.indexing.overlord.HeapMemoryTaskStorage; import org.apache.druid.indexing.overlord.SingleTaskBackgroundRunner; import org.apache.druid.indexing.overlord.TaskRunner; -import org.apache.druid.indexing.overlord.TaskStorage; import org.apache.druid.indexing.seekablestream.SeekableStreamIndexTask; import org.apache.druid.indexing.worker.executor.ExecutorLifecycle; import org.apache.druid.indexing.worker.executor.ExecutorLifecycleConfig; @@ -220,7 +214,6 @@ public void configure(Properties properties) protected List getModules() { return ImmutableList.of( - new MetadataManagerModule(), // needed here only to support druid.peon.mode=local new PeonProcessingModule(), new QueryableModule(), new QueryRunnerFactoryModule(), @@ -485,26 +478,9 @@ static void bindPeonDataSegmentHandlers(Binder binder) private static void configureTaskActionClient(Binder binder) { - PolyBind.createChoice( - binder, - "druid.peon.mode", - Key.get(TaskActionClientFactory.class), - Key.get(RemoteTaskActionClientFactory.class) - ); - final MapBinder taskActionBinder = - PolyBind.optionBinder(binder, Key.get(TaskActionClientFactory.class)); - taskActionBinder - .addBinding("local") - .to(LocalTaskActionClientFactory.class) - .in(LazySingleton.class); - // all of these bindings are so that we can run the peon in local mode - JsonConfigProvider.bind(binder, "druid.indexer.storage", TaskStorageConfig.class); - binder.bind(TaskStorage.class).to(HeapMemoryTaskStorage.class).in(LazySingleton.class); - binder.bind(TaskActionToolbox.class).in(LazySingleton.class); - taskActionBinder - .addBinding("remote") - .to(RemoteTaskActionClientFactory.class) - .in(LazySingleton.class); + binder.bind(TaskActionClientFactory.class) + .to(RemoteTaskActionClientFactory.class) + .in(LazySingleton.class); binder.bind(NodeRole.class).annotatedWith(Self.class).toInstance(NodeRole.PEON); } diff --git a/services/src/main/java/org/apache/druid/cli/ServerRunnable.java b/services/src/main/java/org/apache/druid/cli/ServerRunnable.java index 5faf6e134c60..3dc1711a50e6 100644 --- a/services/src/main/java/org/apache/druid/cli/ServerRunnable.java +++ b/services/src/main/java/org/apache/druid/cli/ServerRunnable.java @@ -33,13 +33,13 @@ import org.apache.druid.error.DruidException; import org.apache.druid.guice.LazySingleton; import org.apache.druid.guice.LifecycleModule; +import org.apache.druid.guice.MetadataConfigModule; import org.apache.druid.guice.ServerViewModule; import org.apache.druid.guice.annotations.Self; import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.java.util.common.lifecycle.Lifecycle; import org.apache.druid.java.util.common.logger.Logger; import org.apache.druid.java.util.emitter.EmittingLogger; -import org.apache.druid.segment.metadata.CentralizedDatasourceSchemaConfig; import org.apache.druid.server.DruidNode; import java.lang.annotation.Annotation; @@ -53,9 +53,6 @@ */ public abstract class ServerRunnable extends GuiceRunnable { - private static final String CENTRALIZED_DATASOURCE_SCHEMA_ENABLED = - CentralizedDatasourceSchemaConfig.PROPERTY_PREFIX + ".enabled"; - private static final EmittingLogger log = new EmittingLogger(ServerRunnable.class); public ServerRunnable(Logger log) @@ -207,7 +204,7 @@ public void stop() protected static void validateCentralizedDatasourceSchemaConfig(Properties properties) { - if (isSegmentSchemaCacheEnabled(properties)) { + if (MetadataConfigModule.isSegmentSchemaCacheEnabled(properties)) { String serverViewType = properties.getProperty(ServerViewModule.SERVERVIEW_TYPE_PROPERTY); if (serverViewType != null && !serverViewType.equals(ServerViewModule.SERVERVIEW_TYPE_HTTP)) { throw DruidException @@ -221,15 +218,10 @@ protected static void validateCentralizedDatasourceSchemaConfig(Properties prope ServerViewModule.SERVERVIEW_TYPE_PROPERTY, serverViewType, ServerViewModule.SERVERVIEW_TYPE_HTTP, - CENTRALIZED_DATASOURCE_SCHEMA_ENABLED + MetadataConfigModule.CENTRALIZED_DATASOURCE_SCHEMA_ENABLED ) ); } } } - - protected static boolean isSegmentSchemaCacheEnabled(Properties properties) - { - return Boolean.parseBoolean(properties.getProperty(CENTRALIZED_DATASOURCE_SCHEMA_ENABLED)); - } } diff --git a/services/src/main/java/org/apache/druid/guice/MetadataManagerModule.java b/services/src/main/java/org/apache/druid/guice/MetadataManagerModule.java index 1cc168318cc2..6d501f9f768c 100644 --- a/services/src/main/java/org/apache/druid/guice/MetadataManagerModule.java +++ b/services/src/main/java/org/apache/druid/guice/MetadataManagerModule.java @@ -27,14 +27,17 @@ import org.apache.druid.indexing.overlord.IndexerMetadataStorageCoordinator; import org.apache.druid.metadata.IndexerSQLMetadataStorageCoordinator; import org.apache.druid.metadata.MetadataRuleManager; +import org.apache.druid.metadata.MetadataRuleManagerConfig; import org.apache.druid.metadata.MetadataRuleManagerProvider; import org.apache.druid.metadata.MetadataSupervisorManager; import org.apache.druid.metadata.SQLMetadataRuleManagerProvider; import org.apache.druid.metadata.SQLMetadataSupervisorManager; import org.apache.druid.metadata.SegmentsMetadataManager; +import org.apache.druid.metadata.SegmentsMetadataManagerConfig; import org.apache.druid.metadata.SegmentsMetadataManagerProvider; import org.apache.druid.metadata.SqlSegmentsMetadataManagerProvider; import org.apache.druid.metadata.segment.SegmentMetadataTransactionFactory; +import org.apache.druid.metadata.segment.SqlSegmentMetadataReadOnlyTransactionFactory; import org.apache.druid.metadata.segment.SqlSegmentMetadataTransactionFactory; import org.apache.druid.metadata.segment.cache.HeapMemorySegmentMetadataCache; import org.apache.druid.metadata.segment.cache.SegmentMetadataCache; @@ -43,43 +46,48 @@ import org.apache.druid.server.coordinator.CoordinatorConfigManager; import org.apache.druid.server.coordinator.MetadataManager; +import java.util.Properties; import java.util.Set; /** - * Module used by Overlord and Coordinator to bind various metadata managers. + * Module used by Overlord and Coordinator to bind the following metadata managers: + *
    + *
  • {@link MetadataManager} - Coordinator only
    • + *
  • {@link MetadataRuleManager} - Coordinator only
    • + *
  • {@link MetadataSupervisorManager}
    • + *
  • {@link SegmentsMetadataManager}
    • + *
  • {@link IndexerMetadataStorageCoordinator}
    • + *
  • {@link CoordinatorConfigManager}
    • + *
  • {@link SegmentMetadataCache}
    • + *
  • {@link SegmentSchemaCache} - Coordinator only
    • + *
*/ public class MetadataManagerModule implements Module { private Set nodeRoles; + private boolean isSchemaCacheEnabled; @Inject public void configure( + Properties properties, @Self Set nodeRoles ) { this.nodeRoles = nodeRoles; + this.isSchemaCacheEnabled = MetadataConfigModule.isSegmentSchemaCacheEnabled(properties); } @Override public void configure(Binder binder) { - if (nodeRoles.contains(NodeRole.COORDINATOR)) { - binder.bind(MetadataRuleManagerProvider.class) - .to(SQLMetadataRuleManagerProvider.class) - .in(LazySingleton.class); - binder.bind(MetadataRuleManager.class) - .toProvider(MetadataRuleManagerProvider.class) - .in(ManageLifecycle.class); - - binder.bind(MetadataManager.class).in(LazySingleton.class); - } - + // Common dependencies binder.bind(CoordinatorConfigManager.class).in(LazySingleton.class); binder.bind(MetadataSupervisorManager.class) .to(SQLMetadataSupervisorManager.class) .in(LazySingleton.class); + JsonConfigProvider.bind(binder, "druid.manager.segments", SegmentsMetadataManagerConfig.class); binder.bind(SegmentsMetadataManagerProvider.class) .to(SqlSegmentsMetadataManagerProvider.class) .in(LazySingleton.class); @@ -87,9 +95,6 @@ public void configure(Binder binder) .toProvider(SegmentsMetadataManagerProvider.class) .in(ManageLifecycle.class); - binder.bind(SegmentMetadataTransactionFactory.class) - .to(SqlSegmentMetadataTransactionFactory.class) - .in(LazySingleton.class); binder.bind(IndexerMetadataStorageCoordinator.class) .to(IndexerSQLMetadataStorageCoordinator.class) .in(ManageLifecycle.class); @@ -97,12 +102,36 @@ public void configure(Binder binder) .to(HeapMemorySegmentMetadataCache.class) .in(LazySingleton.class); + // Coordinator-only dependencies if (nodeRoles.contains(NodeRole.COORDINATOR)) { + JsonConfigProvider.bind(binder, "druid.manager.rules", MetadataRuleManagerConfig.class); + binder.bind(MetadataRuleManagerProvider.class) + .to(SQLMetadataRuleManagerProvider.class) + .in(LazySingleton.class); + binder.bind(MetadataRuleManager.class) + .toProvider(MetadataRuleManagerProvider.class) + .in(ManageLifecycle.class); + + binder.bind(MetadataManager.class).in(LazySingleton.class); + } + + if (nodeRoles.contains(NodeRole.COORDINATOR) && isSchemaCacheEnabled) { binder.bind(SegmentSchemaCache.class).in(LazySingleton.class); } else { binder.bind(SegmentSchemaCache.class) .to(NoopSegmentSchemaCache.class) .in(LazySingleton.class); } + + // Overlord-only dependencies + if (nodeRoles.contains(NodeRole.OVERLORD)) { + binder.bind(SegmentMetadataTransactionFactory.class) + .to(SqlSegmentMetadataTransactionFactory.class) + .in(LazySingleton.class); + } else { + binder.bind(SegmentMetadataTransactionFactory.class) + .to(SqlSegmentMetadataReadOnlyTransactionFactory.class) + .in(LazySingleton.class); + } } } From 52db331e9107f7c765c13d7ccf90557d5786de71 Mon Sep 17 00:00:00 2001 From: Kashif Faraz Date: Sat, 17 May 2025 10:38:06 +0530 Subject: [PATCH 11/18] Fix up tests --- .../druid/indexing/common/actions/TaskActionTestKit.java | 4 ++-- .../apache/druid/indexing/common/task/IngestionTestBase.java | 3 ++- .../apache/druid/segment/metadata/NoopSegmentSchemaCache.java | 3 ++- .../org/apache/druid/segment/metadata/SegmentSchemaCache.java | 1 + .../IndexerSQLMetadataStorageCoordinatorReadOnlyTest.java | 4 ++-- .../metadata/IndexerSQLMetadataStorageCoordinatorTest.java | 4 ++-- .../segment/cache/HeapMemorySegmentMetadataCacheTest.java | 4 ++-- 7 files changed, 13 insertions(+), 10 deletions(-) diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/common/actions/TaskActionTestKit.java b/indexing-service/src/test/java/org/apache/druid/indexing/common/actions/TaskActionTestKit.java index 8efcfa58b1e8..3a63495928aa 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/common/actions/TaskActionTestKit.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/common/actions/TaskActionTestKit.java @@ -39,7 +39,7 @@ import org.apache.druid.metadata.segment.cache.HeapMemorySegmentMetadataCache; import org.apache.druid.metadata.segment.cache.SegmentMetadataCache; import org.apache.druid.segment.metadata.CentralizedDatasourceSchemaConfig; -import org.apache.druid.segment.metadata.SegmentSchemaCache; +import org.apache.druid.segment.metadata.NoopSegmentSchemaCache; import org.apache.druid.segment.metadata.SegmentSchemaManager; import org.apache.druid.server.coordinator.simulate.BlockingExecutorService; import org.apache.druid.server.coordinator.simulate.TestDruidLeaderSelector; @@ -175,7 +175,7 @@ private SqlSegmentMetadataTransactionFactory setupTransactionFactory(ObjectMappe objectMapper, Suppliers.ofInstance(new SegmentsMetadataManagerConfig(Period.seconds(1), cacheMode)), Suppliers.ofInstance(metadataStorageTablesConfig), - new SegmentSchemaCache(), + new NoopSegmentSchemaCache(), testDerbyConnector, (poolSize, name) -> new WrappingScheduledExecutorService(name, metadataCachePollExec, false), NoopServiceEmitter.instance() diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IngestionTestBase.java b/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IngestionTestBase.java index a56d7acc5097..f468f2f04421 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IngestionTestBase.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IngestionTestBase.java @@ -86,6 +86,7 @@ import org.apache.druid.segment.loading.LocalDataSegmentPusherConfig; import org.apache.druid.segment.loading.SegmentCacheManager; import org.apache.druid.segment.metadata.CentralizedDatasourceSchemaConfig; +import org.apache.druid.segment.metadata.NoopSegmentSchemaCache; import org.apache.druid.segment.metadata.SegmentSchemaCache; import org.apache.druid.segment.metadata.SegmentSchemaManager; import org.apache.druid.segment.realtime.NoopChatHandlerProvider; @@ -324,7 +325,7 @@ private SqlSegmentMetadataTransactionFactory createTransactionFactory() objectMapper, Suppliers.ofInstance(new SegmentsMetadataManagerConfig(Period.millis(10), cacheMode)), derbyConnectorRule.metadataTablesConfigSupplier(), - new SegmentSchemaCache(), + new NoopSegmentSchemaCache(), derbyConnectorRule.getConnector(), ScheduledExecutors::fixed, NoopServiceEmitter.instance() diff --git a/server/src/main/java/org/apache/druid/segment/metadata/NoopSegmentSchemaCache.java b/server/src/main/java/org/apache/druid/segment/metadata/NoopSegmentSchemaCache.java index 56dc14e1b91a..7385cbefce0c 100644 --- a/server/src/main/java/org/apache/druid/segment/metadata/NoopSegmentSchemaCache.java +++ b/server/src/main/java/org/apache/druid/segment/metadata/NoopSegmentSchemaCache.java @@ -27,7 +27,8 @@ import java.util.Map; /** - * Noop implementation of {@link SegmentSchemaCache}. + * No-op implementation of {@link SegmentSchemaCache} that always returns false + * for {@link #isEnabled()} and {@link #isInitialized()}. */ public class NoopSegmentSchemaCache extends SegmentSchemaCache { diff --git a/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaCache.java b/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaCache.java index 237b3944a176..c77bdecad022 100644 --- a/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaCache.java +++ b/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaCache.java @@ -99,6 +99,7 @@ public class SegmentSchemaCache */ public boolean isEnabled() { + // Always return true since this implementation is bound only when caching is enabled return true; } diff --git a/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorReadOnlyTest.java b/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorReadOnlyTest.java index b8b72ff72e68..48656aa93e96 100644 --- a/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorReadOnlyTest.java +++ b/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorReadOnlyTest.java @@ -36,7 +36,7 @@ import org.apache.druid.segment.TestDataSource; import org.apache.druid.segment.TestHelper; import org.apache.druid.segment.metadata.CentralizedDatasourceSchemaConfig; -import org.apache.druid.segment.metadata.SegmentSchemaCache; +import org.apache.druid.segment.metadata.NoopSegmentSchemaCache; import org.apache.druid.server.coordinator.simulate.BlockingExecutorService; import org.apache.druid.server.coordinator.simulate.TestDruidLeaderSelector; import org.apache.druid.server.coordinator.simulate.WrappingScheduledExecutorService; @@ -103,7 +103,7 @@ public void setup() mapper, () -> new SegmentsMetadataManagerConfig(null, cacheMode), derbyConnectorRule.metadataTablesConfigSupplier(), - new SegmentSchemaCache(), + new NoopSegmentSchemaCache(), derbyConnector, (corePoolSize, nameFormat) -> new WrappingScheduledExecutorService( nameFormat, diff --git a/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java b/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java index 2562b3733510..3042f53124e5 100644 --- a/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java +++ b/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java @@ -44,7 +44,7 @@ import org.apache.druid.segment.TestDataSource; import org.apache.druid.segment.metadata.CentralizedDatasourceSchemaConfig; import org.apache.druid.segment.metadata.FingerprintGenerator; -import org.apache.druid.segment.metadata.SegmentSchemaCache; +import org.apache.druid.segment.metadata.NoopSegmentSchemaCache; import org.apache.druid.segment.metadata.SegmentSchemaManager; import org.apache.druid.segment.metadata.SegmentSchemaTestUtils; import org.apache.druid.segment.realtime.appenderator.SegmentIdWithShardSpec; @@ -153,7 +153,7 @@ public void setUp() mapper, () -> new SegmentsMetadataManagerConfig(null, cacheMode), derbyConnectorRule.metadataTablesConfigSupplier(), - new SegmentSchemaCache(), + new NoopSegmentSchemaCache(), derbyConnector, (corePoolSize, nameFormat) -> new WrappingScheduledExecutorService( nameFormat, diff --git a/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java b/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java index bdd02ba12d8d..4a79610b5abb 100644 --- a/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java +++ b/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java @@ -34,7 +34,7 @@ import org.apache.druid.metadata.TestDerbyConnector; import org.apache.druid.segment.TestDataSource; import org.apache.druid.segment.TestHelper; -import org.apache.druid.segment.metadata.SegmentSchemaCache; +import org.apache.druid.segment.metadata.NoopSegmentSchemaCache; import org.apache.druid.segment.realtime.appenderator.SegmentIdWithShardSpec; import org.apache.druid.server.coordinator.CreateDataSegments; import org.apache.druid.server.coordinator.simulate.BlockingExecutorService; @@ -105,7 +105,7 @@ private void setupTargetWithCaching(SegmentMetadataCache.UsageMode cacheMode) TestHelper.JSON_MAPPER, () -> metadataManagerConfig, derbyConnectorRule.metadataTablesConfigSupplier(), - new SegmentSchemaCache(), + new NoopSegmentSchemaCache(), derbyConnector, executorFactory, serviceEmitter From 9877493a25ee3add70a5cb56b425ea0cd55f6a53 Mon Sep 17 00:00:00 2001 From: Kashif Faraz Date: Sat, 17 May 2025 14:11:45 +0530 Subject: [PATCH 12/18] Fix logic for delta sync of segment schemas --- .../metadata/SqlSegmentsMetadataQuery.java | 90 +++++++ .../HeapMemoryDatasourceSegmentCache.java | 20 +- .../cache/HeapMemorySegmentMetadataCache.java | 233 +++++++++--------- .../segment/cache/SegmentSchemaRecord.java | 47 ++++ .../segment/cache/SegmentSyncResult.java | 8 +- .../HeapMemorySegmentMetadataCacheTest.java | 2 +- 6 files changed, 276 insertions(+), 124 deletions(-) create mode 100644 server/src/main/java/org/apache/druid/metadata/segment/cache/SegmentSchemaRecord.java diff --git a/server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataQuery.java b/server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataQuery.java index b1d7d16a1453..899b98f9562a 100644 --- a/server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataQuery.java +++ b/server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataQuery.java @@ -39,6 +39,9 @@ import org.apache.druid.java.util.common.jackson.JacksonUtils; import org.apache.druid.java.util.common.logger.Logger; import org.apache.druid.java.util.common.parsers.CloseableIterator; +import org.apache.druid.metadata.segment.cache.SegmentSchemaRecord; +import org.apache.druid.segment.SchemaPayload; +import org.apache.druid.segment.metadata.CentralizedDatasourceSchemaConfig; import org.apache.druid.segment.realtime.appenderator.SegmentIdWithShardSpec; import org.apache.druid.server.http.DataSegmentPlus; import org.apache.druid.timeline.DataSegment; @@ -607,6 +610,71 @@ private CloseableIterator retrieveSegmentBatchById( return CloseableIterators.wrap(resultIterator, resultIterator); } + /** + * Retrieves all used segment schemas present in the metadata store irrespective + * of their last updated time. + */ + public List retrieveAllUsedSegmentSchemas() + { + return retrieveValidSchemaRecordsWithSql( + StringUtils.format( + "SELECT fingerprint, payload FROM %s" + + " WHERE version = %s AND used = true", + dbTables.getSegmentSchemasTable(), CentralizedDatasourceSchemaConfig.SCHEMA_VERSION + ) + ); + } + + /** + * Retrieves segment schemas from the metadata store for the given fingerprints. + */ + public List retrieveUsedSegmentSchemasForFingerprints( + Set schemaFingerprints + ) + { + final List> fingerprintBatches = Lists.partition( + List.copyOf(schemaFingerprints), + MAX_INTERVALS_PER_BATCH + ); + + final List records = new ArrayList<>(); + for (List fingerprintBatch : fingerprintBatches) { + records.addAll( + retrieveBatchOfSegmentSchemas(fingerprintBatch) + ); + } + + return records; + } + + /** + * Retrieves a batch of segment schema records for the given fingerprints. + */ + private List retrieveBatchOfSegmentSchemas(List schemaFingerprint) + { + return retrieveValidSchemaRecordsWithSql( + StringUtils.format( + "SELECT fingerprint, payload FROM %s" + + " WHERE version = %s AND used = true" + + " %s", + dbTables.getSegmentSchemasTable(), + CentralizedDatasourceSchemaConfig.SCHEMA_VERSION, + getParameterizedInConditionForColumn("fingerprint", schemaFingerprint) + ) + ); + } + + private List retrieveValidSchemaRecordsWithSql(String sql) + { + return handle.createQuery(sql) + .setFetchSize(connector.getStreamingFetchSize()) + .map((index, r, ctx) -> mapToSchemaRecord(r)) + .list() + .stream() + .filter(Objects::nonNull) + .collect(Collectors.toList()); + } + /** * Marks the given segment IDs as used. * @@ -1467,6 +1535,28 @@ private Interval mapToInterval(ResultSet resultSet, String dataSource) } } + /** + * Tries to parse the fields of the result set into a {@link SegmentSchemaRecord}. + * + * @return null if an error occurred while parsing the result + */ + @Nullable + private SegmentSchemaRecord mapToSchemaRecord(ResultSet resultSet) + { + String fingerprint = null; + try { + fingerprint = resultSet.getString("fingerprint"); + return new SegmentSchemaRecord( + fingerprint, + jsonMapper.readValue(resultSet.getBytes("payload"), SchemaPayload.class) + ); + } + catch (Throwable t) { + log.error(t, "Could not read segment schema with fingerprint[%s]", fingerprint); + return null; + } + } + private ResultIterator getDataSegmentResultIterator(Query> sql) { return sql.map((index, r, ctx) -> JacksonUtils.readValue(jsonMapper, r.getBytes(2), DataSegment.class)) diff --git a/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemoryDatasourceSegmentCache.java b/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemoryDatasourceSegmentCache.java index 8b9258a32a5f..3ca8ca7d090b 100644 --- a/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemoryDatasourceSegmentCache.java +++ b/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemoryDatasourceSegmentCache.java @@ -168,9 +168,9 @@ SegmentSyncResult syncSegmentIds(List persistedSegments, DateTime // Remove unknown segments from cache final Set persistedSegmentIds = persistedSegments.stream().map(SegmentRecord::getSegmentId).collect(Collectors.toSet()); - final int numSegmentsRemoved = removeUnpersistedSegments(persistedSegmentIds, syncStartTime); + final Set segmentIdsRemoved = removeUnpersistedSegments(persistedSegmentIds, syncStartTime); - return new SegmentSyncResult(numSegmentsRemoved, 0, usedSegmentIdsToRefresh); + return new SegmentSyncResult(segmentIdsRemoved.size(), 0, usedSegmentIdsToRefresh, segmentIdsRemoved); }); } @@ -198,7 +198,7 @@ SegmentSyncResult syncPendingSegments( final Set persistedSegmentIds = persistedPendingSegments.stream().map(s -> s.getId().toString()).collect(Collectors.toSet()); final int numSegmentsRemoved = removeUnpersistedPendingSegments(persistedSegmentIds, syncStartTime); - return new SegmentSyncResult(numSegmentsRemoved, numSegmentsUpdated, Set.of()); + return new SegmentSyncResult(numSegmentsRemoved, numSegmentsUpdated, Set.of(), Set.of()); }); } @@ -224,9 +224,9 @@ && shouldUpdateCache(record.getCreatedDate(), pollStartTime) * * @param persistedSegmentIds Segment IDs present in the metadata store * @param syncStartTime Start time of the current sync - * @return Number of unpersisted segments removed from cache. + * @return Set of unpersisted segment IDs removed from the cache. */ - private int removeUnpersistedSegments(Set persistedSegmentIds, DateTime syncStartTime) + private Set removeUnpersistedSegments(Set persistedSegmentIds, DateTime syncStartTime) { return withWriteLock(() -> { final Set unpersistedSegmentIds = new HashSet<>(); @@ -243,7 +243,9 @@ && shouldUpdateCache(entry.getValue().getUsedStatusLastUpdatedDate(), syncStartT ).map(Map.Entry::getKey).forEach(unpersistedSegmentIds::add); } - return deleteSegments(unpersistedSegmentIds); + deleteSegments(unpersistedSegmentIds); + + return unpersistedSegmentIds; }); } @@ -725,12 +727,6 @@ boolean isEmpty() && unusedSegmentIdToUpdatedTime.isEmpty(); } - private boolean isSegmentIdCached(SegmentId id) - { - return idToUsedSegment.containsKey(id) - || unusedSegmentIdToUpdatedTime.containsKey(id); - } - /** * Removes the given segment from the cache. * diff --git a/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java b/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java index 7cb3b3780273..3bff59f61b31 100644 --- a/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java +++ b/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java @@ -22,6 +22,7 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.base.Supplier; import com.google.common.base.Throwables; +import com.google.common.collect.Sets; import com.google.common.util.concurrent.FutureCallback; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; @@ -106,14 +107,6 @@ public class HeapMemorySegmentMetadataCache implements SegmentMetadataCache private static final int MIN_SYNC_DELAY_MILLIS = 1000; private static final int MAX_IMMEDIATE_SYNC_RETRIES = 3; - /** - * Delta sync fetches only the updates made after {@code previousSyncStartTime - syncWindow}. - * This duration should be enough to account for transaction retries, server - * clocks not being perfectly synced or any other case that might cause past - * records to appear. - */ - private static final Duration DELTA_SYNC_WINDOW = Duration.standardHours(1); - private enum CacheState { STOPPED, FOLLOWER, LEADER_FIRST_SYNC_PENDING, LEADER_FIRST_SYNC_STARTED, LEADER_READY @@ -150,11 +143,7 @@ private enum CacheState private final ConcurrentHashMap datasourceToSegmentCache = new ConcurrentHashMap<>(); - /** - * Start time of the previous delta sync. This can be used to roughly identify - * the latest updated timestamp that is present in the cache. - */ - private final AtomicReference previousSyncStartTime = new AtomicReference<>(); + private final AtomicReference syncFinishTime = new AtomicReference<>(); private final AtomicReference datasourcesSnapshot = new AtomicReference<>(null); @Inject @@ -227,7 +216,7 @@ public void stop() datasourceToSegmentCache.forEach((datasource, cache) -> cache.stop()); datasourceToSegmentCache.clear(); datasourcesSnapshot.set(null); - previousSyncStartTime.set(null); + syncFinishTime.set(null); updateCacheState(CacheState.STOPPED, "Stopped sync with metadata store"); } @@ -287,9 +276,9 @@ public DataSourcesSnapshot getDataSourcesSnapshot() @Override public void awaitNextSync(long timeoutMillis) { - final DateTime lastSyncTime = previousSyncStartTime.get(); + final DateTime lastSyncTime = syncFinishTime.get(); final Supplier lastSyncTimeIsNotUpdated = - () -> Objects.equals(previousSyncStartTime.get(), lastSyncTime); + () -> Objects.equals(syncFinishTime.get(), lastSyncTime); waitForCacheToFinishSyncWhile(lastSyncTimeIsNotUpdated, timeoutMillis); } @@ -528,8 +517,8 @@ public void onFailure(Throwable t) *
  • Retrieve payloads of used segments which have been updated in the metadata * store but not in the cache
    • *
  • Retrieve all pending segments and update the cache as needed.
    • - *
  • If schema caching is enabled, retrieve segment schemas updated since - * the last sync and update them in the {@link SegmentSchemaCache}.
    • + *
  • If schema caching is enabled, retrieve segment schemas and reset them + * in the {@link SegmentSchemaCache}.
    • *
  • Change the cache state to ready if it is leader and waiting for first sync.
    • *
  • Emit metrics
    • * @@ -554,7 +543,7 @@ private long syncWithMetadataStore() final Map datasourceToSummary = new HashMap<>(); // Fetch all used segments if this is the first sync - if (previousSyncStartTime.get() == null) { + if (syncFinishTime.get() == null) { retrieveAllUsedSegments(datasourceToSummary); } else { retrieveUsedSegmentIds(datasourceToSummary); @@ -566,22 +555,13 @@ private long syncWithMetadataStore() retrieveAllPendingSegments(datasourceToSummary); updatePendingSegmentsInCache(datasourceToSummary, syncStartTime); - final DateTime fetchUpdatesAfter = previousSyncStartTime.get(); - if (fetchUpdatesAfter == null) { - retrieveUsedSegmentSchemasUpdatedAfter( - DateTimes.COMPARE_DATE_AS_STRING_MIN, - datasourceToSummary - ); - } else { - retrieveUsedSegmentSchemasUpdatedAfter( - fetchUpdatesAfter.minus(DELTA_SYNC_WINDOW), - datasourceToSummary - ); + if (useSchemaCache) { + retrieveAndResetUsedSegmentSchemas(datasourceToSummary); } markCacheSynced(syncStartTime); - previousSyncStartTime.set(syncStartTime); + syncFinishTime.set(DateTimes.nowUtc()); return totalSyncDuration.millisElapsed(); } @@ -738,6 +718,7 @@ private void updateSegmentIdsInCache( emitNonZeroMetric(dataSource, Metric.DELETED_SEGMENTS, result.getDeleted()); summary.usedSegmentIdsToRefresh.addAll(result.getExpiredIds()); + summary.deletedSegmentIds.addAll(result.getDeletedIds()); }); // Update cache for datasources which returned no segments in the latest poll @@ -745,6 +726,11 @@ private void updateSegmentIdsInCache( if (!datasourceToSummary.containsKey(dataSource)) { final SegmentSyncResult result = cache.syncSegmentIds(List.of(), syncStartTime); emitNonZeroMetric(dataSource, Metric.DELETED_SEGMENTS, result.getDeleted()); + + datasourceToSummary + .computeIfAbsent(dataSource, ds -> new DatasourceSegmentSummary()) + .deletedSegmentIds + .addAll(result.getDeletedIds()); } }); @@ -856,7 +842,7 @@ private void updatePendingSegmentsInCache( final HeapMemoryDatasourceSegmentCache cache = getCacheForDatasource(dataSource); final SegmentSyncResult result = cache.syncPendingSegments(summary.persistedPendingSegments, syncStartTime); - emitMetric(dataSource, Metric.PERSISTED_PENDING_SEGMENTS, summary.persistedPendingSegments.size()); + emitNonZeroMetric(dataSource, Metric.PERSISTED_PENDING_SEGMENTS, summary.persistedPendingSegments.size()); emitNonZeroMetric(dataSource, Metric.UPDATED_PENDING_SEGMENTS, result.getUpdated()); emitNonZeroMetric(dataSource, Metric.DELETED_PENDING_SEGMENTS, result.getDeleted()); }); @@ -922,93 +908,133 @@ private void retrieveAllPendingSegments( } /** - * Retrieves all segment schemas from the metadata store irrespective of their - * used status or last updated time. + * Retrieves required used segment schemas from the metadata store and resets + * them in the {@link SegmentSchemaCache}. */ - private void retrieveUsedSegmentSchemasUpdatedAfter( - DateTime minUpdateTime, + private void retrieveAndResetUsedSegmentSchemas( Map datasourceToSummary ) { - if (!useSchemaCache) { - return; - } + final Stopwatch schemaSyncDuration = Stopwatch.createStarted(); // Emit metrics for the current contents of the cache segmentSchemaCache.getStats().forEach(this::emitMetric); - final Stopwatch schemaSyncDuration = Stopwatch.createStarted(); - final String sql = StringUtils.format( - "SELECT fingerprint, payload FROM %s" - + " WHERE version = %s AND used = true" - + " AND used_status_last_updated >= :minUpdateTime", - tablesConfig.getSegmentSchemasTable(), CentralizedDatasourceSchemaConfig.SCHEMA_VERSION + // Reset the SegmentSchemaCache with latest schemas and metadata + final Map schemaFingerprintToPayload; + if (syncFinishTime.get() == null) { + schemaFingerprintToPayload = buildSchemaFingerprintToPayloadMapForFullSync(); + } else { + schemaFingerprintToPayload = buildSchemaFingerprintToPayloadMapForDeltaSync(); + } + + segmentSchemaCache.resetSchemaForPublishedSegments( + buildSegmentIdToMetadataMapForSync(datasourceToSummary), + schemaFingerprintToPayload ); + emitMetric(Metric.RETRIEVE_SEGMENT_SCHEMAS_DURATION_MILLIS, schemaSyncDuration.millisElapsed()); + } + + /** + * Retrieves all used segment schemas from the metadata store and builds a + * fresh map from schema fingerprint to payload. + */ + private Map buildSchemaFingerprintToPayloadMapForFullSync() + { final List records = inReadOnlyTransaction( - (handle, status) -> - handle.createQuery(sql) - .bind("minUpdateTime", minUpdateTime.toString()) - .setFetchSize(connector.getStreamingFetchSize()) - .map((index, r, ctx) -> mapToSchemaRecord(r)) - .list() + (handle, status) -> SqlSegmentsMetadataQuery + .forHandle(handle, connector, tablesConfig, jsonMapper) + .retrieveAllUsedSegmentSchemas() ); - final Map updatedFingerprintToSchemaPayload = - records.stream().filter(Objects::nonNull).collect(Collectors.toMap(r -> r.fingerprint, r -> r.payload)); - - if (updatedFingerprintToSchemaPayload.size() < records.size()) { - emitMetric(Metric.SKIPPED_SEGMENT_SCHEMAS, records.size() - updatedFingerprintToSchemaPayload.size()); - } + return records.stream().collect( + Collectors.toMap( + SegmentSchemaRecord::getFingerprint, + SegmentSchemaRecord::getPayload + ) + ); + } - // Build a map for the currently cached entries - final Map segmentIdToMetadata = new HashMap<>( - segmentSchemaCache.getSegmentMetadataMap() + /** + * Retrieves segment schemas from the metadata store if they are not present + * in the cache or have been recently updated in the metadata store. These + * segment schemas along with those already present in the cache are used to + * build a complete udpated map from schema fingerprint to payload. + * + * @return Complete updated map from schema fingerprint to payload for all + * used segment schemas currently persisted in the metadata store. + */ + private Map buildSchemaFingerprintToPayloadMapForDeltaSync() + { + // Fetch all used schema fingerprints present in the metadata store + final String sql = StringUtils.format( + "SELECT fingerprint FROM %s WHERE version = %s AND used = true", + tablesConfig.getSegmentSchemasTable(), CentralizedDatasourceSchemaConfig.SCHEMA_VERSION ); - final Map fingerprintToSchemaPayload = new HashMap<>( - segmentSchemaCache.getSchemaPayloadMap() + final List fingerprintRecords = inReadOnlyTransaction( + (handle, status) -> handle.createQuery(sql) + .setFetchSize(connector.getStreamingFetchSize()) + .mapTo(String.class) + .list() ); - // Update the map with the segments fetched in this sync - fingerprintToSchemaPayload.putAll(updatedFingerprintToSchemaPayload); - datasourceToSummary.values().forEach( - summary -> summary.usedSegments.forEach(segment -> { - if (segment.getNumRows() != null && segment.getSchemaFingerprint() != null) { - segmentIdToMetadata.put( - segment.getDataSegment().getId(), - new SegmentMetadata(segment.getNumRows(), segment.getSchemaFingerprint()) - ); - } - }) + // Identify fingerprints in the cache and in the metadata store + final Map schemaFingerprintToPayload = new HashMap<>( + segmentSchemaCache.getSchemaPayloadMap() ); - - segmentSchemaCache.resetSchemaForPublishedSegments( - segmentIdToMetadata, - fingerprintToSchemaPayload + final Set cachedFingerprints = Set.copyOf(schemaFingerprintToPayload.keySet()); + final Set persistedFingerprints = Set.copyOf(fingerprintRecords); + + // Remove entry for schemas that have been deleted from the metadata store + final Set deletedFingerprints = Sets.difference(cachedFingerprints, persistedFingerprints); + deletedFingerprints.forEach(schemaFingerprintToPayload::remove); + + // Retrieve and add entry for schemas that have been added to the metadata store + final Set addedFingerprints = Sets.difference(persistedFingerprints, cachedFingerprints); + final List addedSegmentSchemaRecords = inReadOnlyTransaction( + (handle, status) -> SqlSegmentsMetadataQuery + .forHandle(handle, connector, tablesConfig, jsonMapper) + .retrieveUsedSegmentSchemasForFingerprints(addedFingerprints) ); - emitMetric(Metric.RETRIEVE_SEGMENT_SCHEMAS_DURATION_MILLIS, schemaSyncDuration.millisElapsed()); + if (addedSegmentSchemaRecords.size() < addedFingerprints.size()) { + emitMetric(Metric.SKIPPED_SEGMENT_SCHEMAS, addedFingerprints.size() - addedSegmentSchemaRecords.size()); + } + addedSegmentSchemaRecords.forEach( + schema -> schemaFingerprintToPayload.put(schema.getFingerprint(), schema.getPayload()) + ); + + return schemaFingerprintToPayload; } /** - * Tries to parse the fields of the result set into a {@link SegmentSchemaRecord}. - * - * @return null if an error occurred while parsing the result + * Builds a map from {@link SegmentId} to {@link SegmentMetadata} for all used + * segments currently present in the metadata store based on the current sync. */ - @Nullable - private SegmentSchemaRecord mapToSchemaRecord(ResultSet resultSet) + private Map buildSegmentIdToMetadataMapForSync( + Map datasourceToSummary + ) { - String fingerprint = null; - try { - fingerprint = resultSet.getString("fingerprint"); - return new SegmentSchemaRecord( - fingerprint, - jsonMapper.readValue(resultSet.getBytes("payload"), SchemaPayload.class) - ); - } - catch (Throwable t) { - log.error(t, "Could not read segment schema with fingerprint[%s]", fingerprint); - return null; - } + final Map segmentIdToMetadataMap = new HashMap<>( + segmentSchemaCache.getSegmentMetadataMap() + ); + + datasourceToSummary.values().forEach(summary -> { + // Add entry for segments that have been added to the datasource cache + summary.usedSegments.forEach(segment -> { + if (segment.getNumRows() != null && segment.getSchemaFingerprint() != null) { + segmentIdToMetadataMap.put( + segment.getDataSegment().getId(), + new SegmentMetadata(segment.getNumRows(), segment.getSchemaFingerprint()) + ); + } + }); + + // Remove entry for segments that have been removed from the datasource cache + summary.deletedSegmentIds.forEach(segmentIdToMetadataMap::remove); + }); + + return segmentIdToMetadataMap; } /** @@ -1071,6 +1097,8 @@ private static class DatasourceSegmentSummary final List persistedSegments = new ArrayList<>(); final List persistedPendingSegments = new ArrayList<>(); + final Set deletedSegmentIds = new HashSet<>(); + final Set usedSegmentIdsToRefresh = new HashSet<>(); final Set usedSegments = new HashSet<>(); @@ -1084,19 +1112,4 @@ private void addPendingSegmentRecord(PendingSegmentRecord record) persistedPendingSegments.add(record); } } - - /** - * Represents a single entry in the segment schemas table. - */ - private static class SegmentSchemaRecord - { - private final String fingerprint; - private final SchemaPayload payload; - - private SegmentSchemaRecord(String fingerprint, SchemaPayload payload) - { - this.fingerprint = fingerprint; - this.payload = payload; - } - } } diff --git a/server/src/main/java/org/apache/druid/metadata/segment/cache/SegmentSchemaRecord.java b/server/src/main/java/org/apache/druid/metadata/segment/cache/SegmentSchemaRecord.java new file mode 100644 index 000000000000..b814f1a7fb80 --- /dev/null +++ b/server/src/main/java/org/apache/druid/metadata/segment/cache/SegmentSchemaRecord.java @@ -0,0 +1,47 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.metadata.segment.cache; + +import org.apache.druid.segment.SchemaPayload; + +/** + * Represents a single record in the druid_segmentSchemas table. + */ +public class SegmentSchemaRecord +{ + private final String fingerprint; + private final SchemaPayload payload; + + public SegmentSchemaRecord(String fingerprint, SchemaPayload payload) + { + this.fingerprint = fingerprint; + this.payload = payload; + } + + public String getFingerprint() + { + return fingerprint; + } + + public SchemaPayload getPayload() + { + return payload; + } +} diff --git a/server/src/main/java/org/apache/druid/metadata/segment/cache/SegmentSyncResult.java b/server/src/main/java/org/apache/druid/metadata/segment/cache/SegmentSyncResult.java index 291444ad748f..59d60033e3f8 100644 --- a/server/src/main/java/org/apache/druid/metadata/segment/cache/SegmentSyncResult.java +++ b/server/src/main/java/org/apache/druid/metadata/segment/cache/SegmentSyncResult.java @@ -31,12 +31,14 @@ public class SegmentSyncResult private final int deleted; private final int updated; private final Set expiredIds; + private final Set deletedIds; - public SegmentSyncResult(int deleted, int updated, Set expiredIds) + public SegmentSyncResult(int deleted, int updated, Set expiredIds, Set deletedIds) { this.deleted = deleted; this.updated = updated; this.expiredIds = expiredIds; + this.deletedIds = deletedIds; } public int getDeleted() @@ -54,4 +56,8 @@ public Set getExpiredIds() return expiredIds; } + public Set getDeletedIds() + { + return deletedIds; + } } diff --git a/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java b/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java index 4a79610b5abb..2fc4b9eb0dd6 100644 --- a/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java +++ b/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java @@ -437,7 +437,7 @@ public void testSync_doesNotFail_ifPendingSegmentRecordIsBad() serviceEmitter.verifyValue(Metric.SKIPPED_PENDING_SEGMENTS, 1L); serviceEmitter.verifyValue(Metric.CACHED_USED_SEGMENTS, 1L); serviceEmitter.verifyValue(Metric.PERSISTED_USED_SEGMENTS, 1L); - serviceEmitter.verifyValue(Metric.PERSISTED_PENDING_SEGMENTS, 0L); + serviceEmitter.verifyNotEmitted(Metric.PERSISTED_PENDING_SEGMENTS); Assert.assertEquals( segment.getDataSegment(), From a18a16e9b45612320030cb2dbc2202b84b485d03 Mon Sep 17 00:00:00 2001 From: Kashif Faraz Date: Sun, 18 May 2025 08:59:24 +0530 Subject: [PATCH 13/18] Add tests, fix up logic --- .../metadata/SqlSegmentsMetadataQuery.java | 68 ++++--- .../HeapMemoryDatasourceSegmentCache.java | 14 +- .../cache/HeapMemorySegmentMetadataCache.java | 90 +++++----- .../segment/cache/SegmentSyncResult.java | 9 +- .../metadata/NoopSegmentSchemaCache.java | 4 +- .../segment/metadata/SegmentSchemaCache.java | 16 +- ...SqlMetadataStorageCoordinatorTestBase.java | 51 ++++-- .../HeapMemorySegmentMetadataCacheTest.java | 167 +++++++++++++++++- .../coordinator/CreateDataSegments.java | 18 +- 9 files changed, 322 insertions(+), 115 deletions(-) diff --git a/server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataQuery.java b/server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataQuery.java index 899b98f9562a..de2e35813c4d 100644 --- a/server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataQuery.java +++ b/server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataQuery.java @@ -610,19 +610,35 @@ private CloseableIterator retrieveSegmentBatchById( return CloseableIterators.wrap(resultIterator, resultIterator); } + /** + * Retrieves all used schema fingerprints present in the metadata store. + */ + public Set retrieveAllUsedSegmentSchemaFingerprints() + { + final String sql = StringUtils.format( + "SELECT fingerprint FROM %s WHERE version = %s AND used = true", + dbTables.getSegmentSchemasTable(), CentralizedDatasourceSchemaConfig.SCHEMA_VERSION + ); + return Set.copyOf( + handle.createQuery(sql) + .setFetchSize(connector.getStreamingFetchSize()) + .mapTo(String.class) + .list() + ); + } + /** * Retrieves all used segment schemas present in the metadata store irrespective * of their last updated time. */ public List retrieveAllUsedSegmentSchemas() { - return retrieveValidSchemaRecordsWithSql( - StringUtils.format( - "SELECT fingerprint, payload FROM %s" - + " WHERE version = %s AND used = true", - dbTables.getSegmentSchemasTable(), CentralizedDatasourceSchemaConfig.SCHEMA_VERSION - ) + final String sql = StringUtils.format( + "SELECT fingerprint, payload FROM %s" + + " WHERE version = %s AND used = true", + dbTables.getSegmentSchemasTable(), CentralizedDatasourceSchemaConfig.SCHEMA_VERSION ); + return retrieveValidSchemaRecordsWithQuery(handle.createQuery(sql)); } /** @@ -650,29 +666,33 @@ public List retrieveUsedSegmentSchemasForFingerprints( /** * Retrieves a batch of segment schema records for the given fingerprints. */ - private List retrieveBatchOfSegmentSchemas(List schemaFingerprint) + private List retrieveBatchOfSegmentSchemas(List schemaFingerprints) { - return retrieveValidSchemaRecordsWithSql( - StringUtils.format( - "SELECT fingerprint, payload FROM %s" - + " WHERE version = %s AND used = true" - + " %s", - dbTables.getSegmentSchemasTable(), - CentralizedDatasourceSchemaConfig.SCHEMA_VERSION, - getParameterizedInConditionForColumn("fingerprint", schemaFingerprint) - ) + final String sql = StringUtils.format( + "SELECT fingerprint, payload FROM %s" + + " WHERE version = %s AND used = true" + + " %s", + dbTables.getSegmentSchemasTable(), + CentralizedDatasourceSchemaConfig.SCHEMA_VERSION, + getParameterizedInConditionForColumn("fingerprint", schemaFingerprints) ); + + final Query> query = handle.createQuery(sql); + bindColumnValuesToQueryWithInCondition("fingerprint", schemaFingerprints, query); + + return retrieveValidSchemaRecordsWithQuery(query); } - private List retrieveValidSchemaRecordsWithSql(String sql) + private List retrieveValidSchemaRecordsWithQuery( + Query> query + ) { - return handle.createQuery(sql) - .setFetchSize(connector.getStreamingFetchSize()) - .map((index, r, ctx) -> mapToSchemaRecord(r)) - .list() - .stream() - .filter(Objects::nonNull) - .collect(Collectors.toList()); + return query.setFetchSize(connector.getStreamingFetchSize()) + .map((index, r, ctx) -> mapToSchemaRecord(r)) + .list() + .stream() + .filter(Objects::nonNull) + .collect(Collectors.toList()); } /** diff --git a/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemoryDatasourceSegmentCache.java b/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemoryDatasourceSegmentCache.java index 3ca8ca7d090b..3a091a6c0b47 100644 --- a/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemoryDatasourceSegmentCache.java +++ b/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemoryDatasourceSegmentCache.java @@ -168,9 +168,9 @@ SegmentSyncResult syncSegmentIds(List persistedSegments, DateTime // Remove unknown segments from cache final Set persistedSegmentIds = persistedSegments.stream().map(SegmentRecord::getSegmentId).collect(Collectors.toSet()); - final Set segmentIdsRemoved = removeUnpersistedSegments(persistedSegmentIds, syncStartTime); + final int numSegmentsRemoved = removeUnpersistedSegments(persistedSegmentIds, syncStartTime); - return new SegmentSyncResult(segmentIdsRemoved.size(), 0, usedSegmentIdsToRefresh, segmentIdsRemoved); + return new SegmentSyncResult(numSegmentsRemoved, 0, usedSegmentIdsToRefresh); }); } @@ -198,7 +198,7 @@ SegmentSyncResult syncPendingSegments( final Set persistedSegmentIds = persistedPendingSegments.stream().map(s -> s.getId().toString()).collect(Collectors.toSet()); final int numSegmentsRemoved = removeUnpersistedPendingSegments(persistedSegmentIds, syncStartTime); - return new SegmentSyncResult(numSegmentsRemoved, numSegmentsUpdated, Set.of(), Set.of()); + return new SegmentSyncResult(numSegmentsRemoved, numSegmentsUpdated, Set.of()); }); } @@ -224,9 +224,9 @@ && shouldUpdateCache(record.getCreatedDate(), pollStartTime) * * @param persistedSegmentIds Segment IDs present in the metadata store * @param syncStartTime Start time of the current sync - * @return Set of unpersisted segment IDs removed from the cache. + * @return Number of unpersisted segment IDs removed from the cache. */ - private Set removeUnpersistedSegments(Set persistedSegmentIds, DateTime syncStartTime) + private int removeUnpersistedSegments(Set persistedSegmentIds, DateTime syncStartTime) { return withWriteLock(() -> { final Set unpersistedSegmentIds = new HashSet<>(); @@ -243,9 +243,7 @@ && shouldUpdateCache(entry.getValue().getUsedStatusLastUpdatedDate(), syncStartT ).map(Map.Entry::getKey).forEach(unpersistedSegmentIds::add); } - deleteSegments(unpersistedSegmentIds); - - return unpersistedSegmentIds; + return deleteSegments(unpersistedSegmentIds); }); } diff --git a/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java b/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java index 3bff59f61b31..974ef600ae01 100644 --- a/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java +++ b/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java @@ -51,7 +51,6 @@ import org.apache.druid.query.DruidMetrics; import org.apache.druid.segment.SchemaPayload; import org.apache.druid.segment.SegmentMetadata; -import org.apache.druid.segment.metadata.CentralizedDatasourceSchemaConfig; import org.apache.druid.segment.metadata.SegmentSchemaCache; import org.apache.druid.server.http.DataSegmentPlus; import org.apache.druid.timeline.DataSegment; @@ -76,6 +75,7 @@ import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Function; import java.util.stream.Collectors; /** @@ -515,10 +515,11 @@ public void onFailure(Throwable t) *
  • Retrieve all used segment IDs along with their updated timestamps.
    • *
  • Sync segment IDs in the cache with the retrieved segment IDs.
    • *
  • Retrieve payloads of used segments which have been updated in the metadata - * store but not in the cache
    • + * store but not in the cache. *
  • Retrieve all pending segments and update the cache as needed.
    • *
  • If schema caching is enabled, retrieve segment schemas and reset them * in the {@link SegmentSchemaCache}.
    • + *
  • Clean up entries for datasources which have no segments in the cache anymore.
    • *
  • Change the cache state to ready if it is leader and waiting for first sync.
    • *
  • Emit metrics
    • * @@ -662,6 +663,16 @@ private void retrieveUsedSegmentIds( emitMetric(Metric.RETRIEVE_SEGMENT_IDS_DURATION_MILLIS, retrieveDuration.millisElapsed()); } + private T query(Function sqlFunction) + { + return inReadOnlyTransaction( + (handle, status) -> sqlFunction.apply( + SqlSegmentsMetadataQuery + .forHandle(handle, connector, tablesConfig, jsonMapper) + ) + ); + } + private T inReadOnlyTransaction(TransactionCallback callback) { return connector.retryReadOnlyTransaction(callback, SQL_QUIET_RETRIES, SQL_MAX_RETRIES); @@ -718,7 +729,6 @@ private void updateSegmentIdsInCache( emitNonZeroMetric(dataSource, Metric.DELETED_SEGMENTS, result.getDeleted()); summary.usedSegmentIdsToRefresh.addAll(result.getExpiredIds()); - summary.deletedSegmentIds.addAll(result.getDeletedIds()); }); // Update cache for datasources which returned no segments in the latest poll @@ -726,11 +736,6 @@ private void updateSegmentIdsInCache( if (!datasourceToSummary.containsKey(dataSource)) { final SegmentSyncResult result = cache.syncSegmentIds(List.of(), syncStartTime); emitNonZeroMetric(dataSource, Metric.DELETED_SEGMENTS, result.getDeleted()); - - datasourceToSummary - .computeIfAbsent(dataSource, ds -> new DatasourceSegmentSummary()) - .deletedSegmentIds - .addAll(result.getDeletedIds()); } }); @@ -842,7 +847,7 @@ private void updatePendingSegmentsInCache( final HeapMemoryDatasourceSegmentCache cache = getCacheForDatasource(dataSource); final SegmentSyncResult result = cache.syncPendingSegments(summary.persistedPendingSegments, syncStartTime); - emitNonZeroMetric(dataSource, Metric.PERSISTED_PENDING_SEGMENTS, summary.persistedPendingSegments.size()); + emitMetric(dataSource, Metric.PERSISTED_PENDING_SEGMENTS, summary.persistedPendingSegments.size()); emitNonZeroMetric(dataSource, Metric.UPDATED_PENDING_SEGMENTS, result.getUpdated()); emitNonZeroMetric(dataSource, Metric.DELETED_PENDING_SEGMENTS, result.getDeleted()); }); @@ -909,7 +914,10 @@ private void retrieveAllPendingSegments( /** * Retrieves required used segment schemas from the metadata store and resets - * them in the {@link SegmentSchemaCache}. + * them in the {@link SegmentSchemaCache}. If this is the first sync, all used + * schemas are retrieved from the metadata store. If this is a delta sync, + * first only the fingerprints of all used schemas are retrieved. Payloads are + * then fetched for only the fingerprints which are not present in the cache. */ private void retrieveAndResetUsedSegmentSchemas( Map datasourceToSummary @@ -917,9 +925,6 @@ private void retrieveAndResetUsedSegmentSchemas( { final Stopwatch schemaSyncDuration = Stopwatch.createStarted(); - // Emit metrics for the current contents of the cache - segmentSchemaCache.getStats().forEach(this::emitMetric); - // Reset the SegmentSchemaCache with latest schemas and metadata final Map schemaFingerprintToPayload; if (syncFinishTime.get() == null) { @@ -933,6 +938,8 @@ private void retrieveAndResetUsedSegmentSchemas( schemaFingerprintToPayload ); + // Emit metrics for the current contents of the cache + segmentSchemaCache.getStats().forEach(this::emitMetric); emitMetric(Metric.RETRIEVE_SEGMENT_SCHEMAS_DURATION_MILLIS, schemaSyncDuration.millisElapsed()); } @@ -942,10 +949,8 @@ private void retrieveAndResetUsedSegmentSchemas( */ private Map buildSchemaFingerprintToPayloadMapForFullSync() { - final List records = inReadOnlyTransaction( - (handle, status) -> SqlSegmentsMetadataQuery - .forHandle(handle, connector, tablesConfig, jsonMapper) - .retrieveAllUsedSegmentSchemas() + final List records = query( + SqlSegmentsMetadataQuery::retrieveAllUsedSegmentSchemas ); return records.stream().collect( @@ -967,24 +972,14 @@ private Map buildSchemaFingerprintToPayloadMapForFullSync */ private Map buildSchemaFingerprintToPayloadMapForDeltaSync() { - // Fetch all used schema fingerprints present in the metadata store - final String sql = StringUtils.format( - "SELECT fingerprint FROM %s WHERE version = %s AND used = true", - tablesConfig.getSegmentSchemasTable(), CentralizedDatasourceSchemaConfig.SCHEMA_VERSION - ); - final List fingerprintRecords = inReadOnlyTransaction( - (handle, status) -> handle.createQuery(sql) - .setFetchSize(connector.getStreamingFetchSize()) - .mapTo(String.class) - .list() - ); - // Identify fingerprints in the cache and in the metadata store final Map schemaFingerprintToPayload = new HashMap<>( - segmentSchemaCache.getSchemaPayloadMap() + segmentSchemaCache.getPublishedSchemaPayloadMap() ); final Set cachedFingerprints = Set.copyOf(schemaFingerprintToPayload.keySet()); - final Set persistedFingerprints = Set.copyOf(fingerprintRecords); + final Set persistedFingerprints = query( + SqlSegmentsMetadataQuery::retrieveAllUsedSegmentSchemaFingerprints + ); // Remove entry for schemas that have been deleted from the metadata store final Set deletedFingerprints = Sets.difference(cachedFingerprints, persistedFingerprints); @@ -992,10 +987,8 @@ private Map buildSchemaFingerprintToPayloadMapForDeltaSyn // Retrieve and add entry for schemas that have been added to the metadata store final Set addedFingerprints = Sets.difference(persistedFingerprints, cachedFingerprints); - final List addedSegmentSchemaRecords = inReadOnlyTransaction( - (handle, status) -> SqlSegmentsMetadataQuery - .forHandle(handle, connector, tablesConfig, jsonMapper) - .retrieveUsedSegmentSchemasForFingerprints(addedFingerprints) + final List addedSegmentSchemaRecords = query( + sql -> sql.retrieveUsedSegmentSchemasForFingerprints(addedFingerprints) ); if (addedSegmentSchemaRecords.size() < addedFingerprints.size()) { emitMetric(Metric.SKIPPED_SEGMENT_SCHEMAS, addedFingerprints.size() - addedSegmentSchemaRecords.size()); @@ -1015,26 +1008,35 @@ private Map buildSegmentIdToMetadataMapForSync( Map datasourceToSummary ) { - final Map segmentIdToMetadataMap = new HashMap<>( - segmentSchemaCache.getSegmentMetadataMap() + final Map cachedSegmentIdToMetadata = + segmentSchemaCache.getPublishedSegmentMetadataMap(); + + final Map syncedSegmentIdToMetadataMap = new HashMap<>( + cachedSegmentIdToMetadata ); + // Remove entry for segments not present in datasource cache (now synced with the metadata store) + cachedSegmentIdToMetadata.keySet().forEach(segmentId -> { + final DataSegment cachedSegment = + getCacheForDatasource(segmentId.getDataSource()).findUsedSegment(segmentId); + if (cachedSegment == null) { + syncedSegmentIdToMetadataMap.remove(segmentId); + } + }); + + // Add entry for segments that have been added to the datasource cache datasourceToSummary.values().forEach(summary -> { - // Add entry for segments that have been added to the datasource cache summary.usedSegments.forEach(segment -> { if (segment.getNumRows() != null && segment.getSchemaFingerprint() != null) { - segmentIdToMetadataMap.put( + syncedSegmentIdToMetadataMap.put( segment.getDataSegment().getId(), new SegmentMetadata(segment.getNumRows(), segment.getSchemaFingerprint()) ); } }); - - // Remove entry for segments that have been removed from the datasource cache - summary.deletedSegmentIds.forEach(segmentIdToMetadataMap::remove); }); - return segmentIdToMetadataMap; + return syncedSegmentIdToMetadataMap; } /** @@ -1097,8 +1099,6 @@ private static class DatasourceSegmentSummary final List persistedSegments = new ArrayList<>(); final List persistedPendingSegments = new ArrayList<>(); - final Set deletedSegmentIds = new HashSet<>(); - final Set usedSegmentIdsToRefresh = new HashSet<>(); final Set usedSegments = new HashSet<>(); diff --git a/server/src/main/java/org/apache/druid/metadata/segment/cache/SegmentSyncResult.java b/server/src/main/java/org/apache/druid/metadata/segment/cache/SegmentSyncResult.java index 59d60033e3f8..90de50f5b478 100644 --- a/server/src/main/java/org/apache/druid/metadata/segment/cache/SegmentSyncResult.java +++ b/server/src/main/java/org/apache/druid/metadata/segment/cache/SegmentSyncResult.java @@ -31,14 +31,12 @@ public class SegmentSyncResult private final int deleted; private final int updated; private final Set expiredIds; - private final Set deletedIds; - public SegmentSyncResult(int deleted, int updated, Set expiredIds, Set deletedIds) + public SegmentSyncResult(int deleted, int updated, Set expiredIds) { this.deleted = deleted; this.updated = updated; this.expiredIds = expiredIds; - this.deletedIds = deletedIds; } public int getDeleted() @@ -55,9 +53,4 @@ public Set getExpiredIds() { return expiredIds; } - - public Set getDeletedIds() - { - return deletedIds; - } } diff --git a/server/src/main/java/org/apache/druid/segment/metadata/NoopSegmentSchemaCache.java b/server/src/main/java/org/apache/druid/segment/metadata/NoopSegmentSchemaCache.java index 7385cbefce0c..c58c1f972091 100644 --- a/server/src/main/java/org/apache/druid/segment/metadata/NoopSegmentSchemaCache.java +++ b/server/src/main/java/org/apache/druid/segment/metadata/NoopSegmentSchemaCache.java @@ -90,7 +90,7 @@ public void markSchemaPersisted(SegmentId segmentId) } @Override - public Map getSegmentMetadataMap() + public Map getPublishedSegmentMetadataMap() { throw new UnsupportedOperationException(); } @@ -102,7 +102,7 @@ public boolean isSchemaCached(SegmentId segmentId) } @Override - public Map getSchemaPayloadMap() + public Map getPublishedSchemaPayloadMap() { throw new UnsupportedOperationException(); } diff --git a/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaCache.java b/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaCache.java index c77bdecad022..79276c70a920 100644 --- a/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaCache.java +++ b/server/src/main/java/org/apache/druid/segment/metadata/SegmentSchemaCache.java @@ -233,9 +233,9 @@ public Optional getSchemaForSegment(SegmentId segmentId) } // segment schema has been polled from the DB - SegmentMetadata segmentMetadata = getSegmentMetadataMap().get(segmentId); + SegmentMetadata segmentMetadata = getPublishedSegmentMetadataMap().get(segmentId); if (segmentMetadata != null) { - SchemaPayload schemaPayload = getSchemaPayloadMap().get(segmentMetadata.getSchemaFingerprint()); + SchemaPayload schemaPayload = getPublishedSchemaPayloadMap().get(segmentMetadata.getSchemaFingerprint()); if (schemaPayload != null) { return Optional.of( new SchemaPayloadPlus(schemaPayload, segmentMetadata.getNumRows()) @@ -260,9 +260,9 @@ public boolean isSchemaCached(SegmentId segmentId) private boolean isPublishedSegmentSchemaCached(SegmentId segmentId) { - SegmentMetadata segmentMetadata = getSegmentMetadataMap().get(segmentId); + SegmentMetadata segmentMetadata = getPublishedSegmentMetadataMap().get(segmentId); if (segmentMetadata != null) { - return getSchemaPayloadMap().containsKey(segmentMetadata.getSchemaFingerprint()); + return getPublishedSchemaPayloadMap().containsKey(segmentMetadata.getSchemaFingerprint()); } return false; } @@ -271,7 +271,7 @@ private boolean isPublishedSegmentSchemaCached(SegmentId segmentId) * @return Immutable map from segment ID to {@link SegmentMetadata} for all * published used segments currently present in this cache. */ - public Map getSegmentMetadataMap() + public Map getPublishedSegmentMetadataMap() { return publishedSegmentSchemas.get().segmentIdToMetadata; } @@ -280,7 +280,7 @@ public Map getSegmentMetadataMap() * @return Immutable map from schema fingerprint to {@link SchemaPayload} for * all schema fingerprints currently present in this cache. */ - public Map getSchemaPayloadMap() + public Map getPublishedSchemaPayloadMap() { return publishedSegmentSchemas.get().schemaFingerprintToPayload; } @@ -315,8 +315,8 @@ public Map getStats() return Map.of( Metric.CACHE_MISSES, cacheMissCount.getAndSet(0), Metric.REALTIME_SEGMENT_SCHEMAS, realtimeSegmentSchemas.size(), - Metric.USED_SEGMENT_SCHEMAS, getSegmentMetadataMap().size(), - Metric.USED_SEGMENT_SCHEMA_FINGERPRINTS, getSchemaPayloadMap().size(), + Metric.USED_SEGMENT_SCHEMAS, getPublishedSegmentMetadataMap().size(), + Metric.USED_SEGMENT_SCHEMA_FINGERPRINTS, getPublishedSchemaPayloadMap().size(), Metric.SCHEMAS_PENDING_BACKFILL, schemasPendingBackfill.size() ); } diff --git a/server/src/test/java/org/apache/druid/metadata/IndexerSqlMetadataStorageCoordinatorTestBase.java b/server/src/test/java/org/apache/druid/metadata/IndexerSqlMetadataStorageCoordinatorTestBase.java index a5e30ae90a74..c6abbce7a12e 100644 --- a/server/src/test/java/org/apache/druid/metadata/IndexerSqlMetadataStorageCoordinatorTestBase.java +++ b/server/src/test/java/org/apache/druid/metadata/IndexerSqlMetadataStorageCoordinatorTestBase.java @@ -46,10 +46,12 @@ import org.apache.druid.timeline.partition.NumberedShardSpec; import org.apache.druid.timeline.partition.ShardSpec; import org.apache.druid.timeline.partition.TombstoneShardSpec; +import org.jetbrains.annotations.NotNull; import org.joda.time.DateTime; import org.joda.time.Interval; import org.junit.Assert; import org.skife.jdbi.v2.PreparedBatch; +import org.skife.jdbi.v2.PreparedBatchPart; import org.skife.jdbi.v2.ResultIterator; import org.skife.jdbi.v2.util.StringMapper; @@ -578,11 +580,12 @@ public static void insertUsedSegments( ); } - insertSegments(usedSegments, derbyConnectorRule, jsonMapper); + insertSegments(usedSegments, false, derbyConnectorRule, jsonMapper); } public static void insertSegments( Set dataSegments, + boolean includeSchema, TestDerbyConnector.DerbyConnectorRule derbyConnectorRule, ObjectMapper jsonMapper ) @@ -590,23 +593,15 @@ public static void insertSegments( final TestDerbyConnector connector = derbyConnectorRule.getConnector(); final String table = derbyConnectorRule.metadataTablesConfigSupplier().get().getSegmentsTable(); + final String sql = getSegmentInsertSql(includeSchema, table, connector); connector.retryWithHandle( handle -> { - PreparedBatch preparedBatch = handle.prepareBatch( - StringUtils.format( - "INSERT INTO %1$s (id, dataSource, created_date, start, %2$send%2$s, partitioned, version," - + " used, payload, used_status_last_updated, upgraded_from_segment_id) " - + "VALUES (:id, :dataSource, :created_date, :start, :end, :partitioned, :version," - + " :used, :payload, :used_status_last_updated, :upgraded_from_segment_id)", - table, - connector.getQuoteString() - ) - ); + PreparedBatch preparedBatch = handle.prepareBatch(sql); for (DataSegmentPlus segmentPlus : dataSegments) { final DataSegment segment = segmentPlus.getDataSegment(); String id = segment.getId().toString(); - preparedBatch.add() - .bind("id", id) + final PreparedBatchPart segmentRecord = preparedBatch.add(); + segmentRecord.bind("id", id) .bind("dataSource", segment.getDataSource()) .bind("created_date", nullSafeString(segmentPlus.getCreatedDate())) .bind("start", segment.getInterval().getStart().toString()) @@ -617,6 +612,11 @@ public static void insertSegments( .bind("payload", jsonMapper.writeValueAsBytes(segment)) .bind("used_status_last_updated", nullSafeString(segmentPlus.getUsedStatusLastUpdatedDate())) .bind("upgraded_from_segment_id", segmentPlus.getUpgradedFromSegmentId()); + + if (includeSchema) { + segmentRecord.bind("num_rows", segmentPlus.getNumRows()) + .bind("schema_fingerprint", segmentPlus.getSchemaFingerprint()); + } } final int[] affectedRows = preparedBatch.execute(); @@ -629,6 +629,31 @@ public static void insertSegments( ); } + private static @NotNull String getSegmentInsertSql(boolean includeSchema, String table, TestDerbyConnector connector) + { + final String sql; + if (includeSchema) { + sql = StringUtils.format( + "INSERT INTO %1$s (id, dataSource, created_date, start, %2$send%2$s, partitioned, version," + + " used, payload, used_status_last_updated, upgraded_from_segment_id, num_rows, schema_fingerprint) " + + "VALUES (:id, :dataSource, :created_date, :start, :end, :partitioned, :version," + + " :used, :payload, :used_status_last_updated, :upgraded_from_segment_id, :num_rows, :schema_fingerprint)", + table, + connector.getQuoteString() + ); + } else { + sql = StringUtils.format( + "INSERT INTO %1$s (id, dataSource, created_date, start, %2$send%2$s, partitioned, version," + + " used, payload, used_status_last_updated, upgraded_from_segment_id) " + + "VALUES (:id, :dataSource, :created_date, :start, :end, :partitioned, :version," + + " :used, :payload, :used_status_last_updated, :upgraded_from_segment_id)", + table, + connector.getQuoteString() + ); + } + return sql; + } + @Nullable private static String nullSafeString(DateTime date) { diff --git a/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java b/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java index 2fc4b9eb0dd6..2a5940b04951 100644 --- a/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java +++ b/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java @@ -32,9 +32,16 @@ import org.apache.druid.metadata.PendingSegmentRecord; import org.apache.druid.metadata.SegmentsMetadataManagerConfig; import org.apache.druid.metadata.TestDerbyConnector; +import org.apache.druid.segment.SchemaPayload; +import org.apache.druid.segment.SegmentMetadata; import org.apache.druid.segment.TestDataSource; import org.apache.druid.segment.TestHelper; +import org.apache.druid.segment.column.RowSignature; +import org.apache.druid.segment.metadata.CentralizedDatasourceSchemaConfig; +import org.apache.druid.segment.metadata.FingerprintGenerator; import org.apache.druid.segment.metadata.NoopSegmentSchemaCache; +import org.apache.druid.segment.metadata.SegmentSchemaCache; +import org.apache.druid.segment.metadata.SegmentSchemaTestUtils; import org.apache.druid.segment.realtime.appenderator.SegmentIdWithShardSpec; import org.apache.druid.server.coordinator.CreateDataSegments; import org.apache.druid.server.coordinator.simulate.BlockingExecutorService; @@ -53,13 +60,14 @@ import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.List; +import java.util.Map; import java.util.Set; public class HeapMemorySegmentMetadataCacheTest { @Rule public final TestDerbyConnector.DerbyConnectorRule derbyConnectorRule - = new TestDerbyConnector.DerbyConnectorRule(); + = new TestDerbyConnector.DerbyConnectorRule(CentralizedDatasourceSchemaConfig.enabled(true)); private BlockingExecutorService pollExecutor; private ScheduledExecutorFactory executorFactory; @@ -67,6 +75,8 @@ public class HeapMemorySegmentMetadataCacheTest private StubServiceEmitter serviceEmitter; private HeapMemorySegmentMetadataCache cache; + private SegmentSchemaCache schemaCache; + private SegmentSchemaTestUtils schemaTestUtils; @Before public void setup() @@ -77,8 +87,10 @@ public void setup() serviceEmitter = new StubServiceEmitter(); derbyConnector.createSegmentTable(); + derbyConnector.createSegmentSchemasTable(); derbyConnector.createPendingSegmentsTable(); + schemaTestUtils = new SegmentSchemaTestUtils(derbyConnectorRule, derbyConnector, TestHelper.JSON_MAPPER); EmittingLogger.registerEmitter(serviceEmitter); } @@ -91,27 +103,41 @@ public void tearDown() } } + private void setupTargetWithCaching(SegmentMetadataCache.UsageMode cacheMode) + { + setupTargetWithCaching(cacheMode, false); + } + /** * Creates the target {@link #cache} to be tested in the current test. */ - private void setupTargetWithCaching(SegmentMetadataCache.UsageMode cacheMode) + private void setupTargetWithCaching(SegmentMetadataCache.UsageMode cacheMode, boolean useSchemaCache) { if (cache != null) { throw new ISE("Test target has already been initialized with caching[%s]", cache.isEnabled()); } final SegmentsMetadataManagerConfig metadataManagerConfig = new SegmentsMetadataManagerConfig(null, cacheMode); + schemaCache = useSchemaCache ? new SegmentSchemaCache() : new NoopSegmentSchemaCache(); cache = new HeapMemorySegmentMetadataCache( TestHelper.JSON_MAPPER, () -> metadataManagerConfig, derbyConnectorRule.metadataTablesConfigSupplier(), - new NoopSegmentSchemaCache(), + schemaCache, derbyConnector, executorFactory, serviceEmitter ); } + private void setupAndSyncCacheWithSchema() + { + setupTargetWithCaching(SegmentMetadataCache.UsageMode.ALWAYS, true); + cache.start(); + cache.becomeLeader(); + syncCacheAfterBecomingLeader(); + } + private void setupAndSyncCache() { setupTargetWithCaching(SegmentMetadataCache.UsageMode.ALWAYS); @@ -437,7 +463,7 @@ public void testSync_doesNotFail_ifPendingSegmentRecordIsBad() serviceEmitter.verifyValue(Metric.SKIPPED_PENDING_SEGMENTS, 1L); serviceEmitter.verifyValue(Metric.CACHED_USED_SEGMENTS, 1L); serviceEmitter.verifyValue(Metric.PERSISTED_USED_SEGMENTS, 1L); - serviceEmitter.verifyNotEmitted(Metric.PERSISTED_PENDING_SEGMENTS); + serviceEmitter.verifyValue(Metric.PERSISTED_PENDING_SEGMENTS, 0L); Assert.assertEquals( segment.getDataSegment(), @@ -694,10 +720,141 @@ public void testSync_cleansUpDataSourceCache_ifEmptyAndNotInUse() serviceEmitter.verifyValue(Metric.DELETED_DATASOURCES, 1L); } + @Test + public void test_sync_addsUsedSegmentSchema_ifNotPresentInCache() + { + setupAndSyncCacheWithSchema(); + + Assert.assertTrue( + schemaCache.getPublishedSchemaPayloadMap().isEmpty() + ); + + final SchemaPayload payload = new SchemaPayload( + RowSignature.builder().add("col1", null).build() + ); + final String fingerprint = getSchemaFingerprint(payload); + schemaTestUtils.insertSegmentSchema( + TestDataSource.WIKI, + Map.of(fingerprint, payload), + Set.of(fingerprint) + ); + + syncCache(); + serviceEmitter.verifyValue("segment/schemaCache/usedFingerprint/count", 1L); + + Assert.assertEquals( + Map.of(fingerprint, payload), + schemaCache.getPublishedSchemaPayloadMap() + ); + } + + @Test + public void test_sync_removesUsedSegmentSchema_ifNotPresentInMetadataStore() + { + setupAndSyncCacheWithSchema(); + + final SchemaPayload payload = new SchemaPayload( + RowSignature.builder().add("col1", null).build() + ); + final String fingerprint = getSchemaFingerprint(payload); + + schemaCache.resetSchemaForPublishedSegments( + Map.of(), + Map.of(fingerprint, payload) + ); + Assert.assertEquals( + Map.of(fingerprint, payload), + schemaCache.getPublishedSchemaPayloadMap() + ); + + syncCache(); + serviceEmitter.verifyValue("segment/schemaCache/usedFingerprint/count", 0L); + + Assert.assertTrue( + schemaCache.getPublishedSchemaPayloadMap().isEmpty() + ); + } + + @Test + public void test_sync_addsUsedSegmentMetadata_ifNotPresentInCache() + { + setupAndSyncCacheWithSchema(); + + Assert.assertTrue( + schemaCache.getPublishedSegmentMetadataMap().isEmpty() + ); + + final SchemaPayload payload = new SchemaPayload( + RowSignature.builder().add("col1", null).build() + ); + final String fingerprint = getSchemaFingerprint(payload); + + final DataSegmentPlus usedSegmentPlus + = CreateDataSegments.ofDatasource(TestDataSource.WIKI) + .withNumRows(10L).withSchemaFingerprint(fingerprint) + .updatedNow().markUsed().asPlus(); + insertSegmentsInMetadataStoreWithSchema(usedSegmentPlus); + + syncCache(); + serviceEmitter.verifyValue(Metric.PERSISTED_USED_SEGMENTS, 1L); + serviceEmitter.verifyValue(Metric.CACHED_USED_SEGMENTS, 1L); + serviceEmitter.verifyValue(Metric.UPDATED_USED_SEGMENTS, 1L); + serviceEmitter.verifyValue("segment/schemaCache/used/count", 1L); + + Assert.assertEquals( + Map.of(usedSegmentPlus.getDataSegment().getId(), new SegmentMetadata(10L, fingerprint)), + schemaCache.getPublishedSegmentMetadataMap() + ); + } + + @Test + public void test_sync_removesUsedSegmentMetadata_ifNotPresentInMetadataStore() + { + setupAndSyncCacheWithSchema(); + + final SchemaPayload payload = new SchemaPayload( + RowSignature.builder().add("col1", null).build() + ); + final String fingerprint = getSchemaFingerprint(payload); + final SegmentId segmentId = SegmentId.dummy(TestDataSource.WIKI); + final SegmentMetadata metadata = new SegmentMetadata(10L, fingerprint); + + schemaCache.resetSchemaForPublishedSegments( + Map.of(segmentId, metadata), + Map.of() + ); + Assert.assertEquals( + Map.of(segmentId, metadata), + schemaCache.getPublishedSegmentMetadataMap() + ); + + syncCache(); + serviceEmitter.verifyValue("segment/schemaCache/used/count", 0L); + + Assert.assertTrue( + schemaCache.getPublishedSegmentMetadataMap().isEmpty() + ); + } + + private static String getSchemaFingerprint(SchemaPayload payload) + { + return new FingerprintGenerator(TestHelper.JSON_MAPPER).generateFingerprint( + payload, + TestDataSource.WIKI, + CentralizedDatasourceSchemaConfig.SCHEMA_VERSION + ); + } + private void insertSegmentsInMetadataStore(Set segments) { IndexerSqlMetadataStorageCoordinatorTestBase - .insertSegments(segments, derbyConnectorRule, TestHelper.JSON_MAPPER); + .insertSegments(segments, false, derbyConnectorRule, TestHelper.JSON_MAPPER); + } + + private void insertSegmentsInMetadataStoreWithSchema(DataSegmentPlus... segments) + { + IndexerSqlMetadataStorageCoordinatorTestBase + .insertSegments(Set.of(segments), true, derbyConnectorRule, TestHelper.JSON_MAPPER); } private void updateSegmentInMetadataStore(DataSegmentPlus segment) diff --git a/server/src/test/java/org/apache/druid/server/coordinator/CreateDataSegments.java b/server/src/test/java/org/apache/druid/server/coordinator/CreateDataSegments.java index b5775409e69d..ab3c6efcee1d 100644 --- a/server/src/test/java/org/apache/druid/server/coordinator/CreateDataSegments.java +++ b/server/src/test/java/org/apache/druid/server/coordinator/CreateDataSegments.java @@ -61,6 +61,8 @@ public class CreateDataSegments private Boolean used; private DateTime lastUpdatedTime; private String upgradedFromSegmentId; + private String schemaFingerprint; + private Long numRows; public static CreateDataSegments ofDatasource(String datasource) { @@ -109,6 +111,18 @@ public CreateDataSegments withVersion(String version) return this; } + public CreateDataSegments withNumRows(Long numRows) + { + this.numRows = numRows; + return this; + } + + public CreateDataSegments withSchemaFingerprint(String schemaFingerprint) + { + this.schemaFingerprint = schemaFingerprint; + return this; + } + public CreateDataSegments markUnused() { this.used = false; @@ -186,8 +200,8 @@ private DataSegmentPlus plus(DataSegment segment) DateTimes.nowUtc(), lastUpdatedTime, used, - null, - null, + schemaFingerprint, + numRows, upgradedFromSegmentId ); } From c4e37abed583d7d26d8b5891bd290e5b98ca9993 Mon Sep 17 00:00:00 2001 From: Kashif Faraz Date: Sun, 18 May 2025 10:06:31 +0530 Subject: [PATCH 14/18] Remove extra changes --- .../apache/druid/metadata/segment/cache/SegmentSyncResult.java | 1 + .../metadata/IndexerSqlMetadataStorageCoordinatorTestBase.java | 3 +-- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/apache/druid/metadata/segment/cache/SegmentSyncResult.java b/server/src/main/java/org/apache/druid/metadata/segment/cache/SegmentSyncResult.java index 90de50f5b478..291444ad748f 100644 --- a/server/src/main/java/org/apache/druid/metadata/segment/cache/SegmentSyncResult.java +++ b/server/src/main/java/org/apache/druid/metadata/segment/cache/SegmentSyncResult.java @@ -53,4 +53,5 @@ public Set getExpiredIds() { return expiredIds; } + } diff --git a/server/src/test/java/org/apache/druid/metadata/IndexerSqlMetadataStorageCoordinatorTestBase.java b/server/src/test/java/org/apache/druid/metadata/IndexerSqlMetadataStorageCoordinatorTestBase.java index c6abbce7a12e..17a02f19a732 100644 --- a/server/src/test/java/org/apache/druid/metadata/IndexerSqlMetadataStorageCoordinatorTestBase.java +++ b/server/src/test/java/org/apache/druid/metadata/IndexerSqlMetadataStorageCoordinatorTestBase.java @@ -46,7 +46,6 @@ import org.apache.druid.timeline.partition.NumberedShardSpec; import org.apache.druid.timeline.partition.ShardSpec; import org.apache.druid.timeline.partition.TombstoneShardSpec; -import org.jetbrains.annotations.NotNull; import org.joda.time.DateTime; import org.joda.time.Interval; import org.junit.Assert; @@ -629,7 +628,7 @@ public static void insertSegments( ); } - private static @NotNull String getSegmentInsertSql(boolean includeSchema, String table, TestDerbyConnector connector) + private static String getSegmentInsertSql(boolean includeSchema, String table, TestDerbyConnector connector) { final String sql; if (includeSchema) { From 368b0e6d1a5bae1f98b1e2c5de5b454da721130e Mon Sep 17 00:00:00 2001 From: Kashif Faraz Date: Mon, 19 May 2025 10:25:53 +0530 Subject: [PATCH 15/18] Attempt to fix test --- .../druid/indexing/common/task/IngestionTestBase.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IngestionTestBase.java b/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IngestionTestBase.java index f468f2f04421..848a2bb91ce5 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IngestionTestBase.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IngestionTestBase.java @@ -86,7 +86,6 @@ import org.apache.druid.segment.loading.LocalDataSegmentPusherConfig; import org.apache.druid.segment.loading.SegmentCacheManager; import org.apache.druid.segment.metadata.CentralizedDatasourceSchemaConfig; -import org.apache.druid.segment.metadata.NoopSegmentSchemaCache; import org.apache.druid.segment.metadata.SegmentSchemaCache; import org.apache.druid.segment.metadata.SegmentSchemaManager; import org.apache.druid.segment.realtime.NoopChatHandlerProvider; @@ -136,6 +135,7 @@ public abstract class IngestionTestBase extends InitializedNullHandlingTest private SupervisorManager supervisorManager; private TestDataSegmentKiller dataSegmentKiller; private SegmentMetadataCache segmentMetadataCache; + private SegmentSchemaCache segmentSchemaCache; protected File reportsFile; protected IngestionTestBase() @@ -168,6 +168,7 @@ public void setUpIngestionTestBase() throws IOException derbyConnectorRule.getConnector() ); + segmentSchemaCache = new SegmentSchemaCache(); storageCoordinator = new IndexerSQLMetadataStorageCoordinator( createTransactionFactory(), objectMapper, @@ -176,7 +177,6 @@ public void setUpIngestionTestBase() throws IOException segmentSchemaManager, CentralizedDatasourceSchemaConfig.create() ); - SegmentSchemaCache segmentSchemaCache = new SegmentSchemaCache(); segmentsMetadataManager = new SqlSegmentsMetadataManagerV2( segmentMetadataCache, segmentSchemaCache, @@ -325,7 +325,7 @@ private SqlSegmentMetadataTransactionFactory createTransactionFactory() objectMapper, Suppliers.ofInstance(new SegmentsMetadataManagerConfig(Period.millis(10), cacheMode)), derbyConnectorRule.metadataTablesConfigSupplier(), - new NoopSegmentSchemaCache(), + segmentSchemaCache, derbyConnectorRule.getConnector(), ScheduledExecutors::fixed, NoopServiceEmitter.instance() From e274065410c5d5eacca95c83f5c2a9f8d408e1cf Mon Sep 17 00:00:00 2001 From: Kashif Faraz Date: Mon, 19 May 2025 19:45:36 +0530 Subject: [PATCH 16/18] Handle race conditions --- .../cache/HeapMemorySegmentMetadataCache.java | 16 ++++++++++++++-- .../HeapMemorySegmentMetadataCacheTest.java | 2 +- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java b/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java index 974ef600ae01..0c3dd4c2f786 100644 --- a/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java +++ b/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java @@ -107,6 +107,18 @@ public class HeapMemorySegmentMetadataCache implements SegmentMetadataCache private static final int MIN_SYNC_DELAY_MILLIS = 1000; private static final int MAX_IMMEDIATE_SYNC_RETRIES = 3; + /** + * Buffer duration for which entries are kept in the cache even if the + * metadata store does not have them. In other words, a segment entry is + * removed from cache if the entry is not present in metadata store and has a + * {@code lastUpdatedTime < now - bufferWindow}. + *

    + * This is primarily needed to handle a race condition between insert and sync + * where an entry with an updated time just before the sync start is added to + * the cache just after the sync has started. + */ + private static final Duration SYNC_BUFFER_DURATION = Duration.standardMinutes(10); + private enum CacheState { STOPPED, FOLLOWER, LEADER_FIRST_SYNC_PENDING, LEADER_FIRST_SYNC_STARTED, LEADER_READY @@ -548,13 +560,13 @@ private long syncWithMetadataStore() retrieveAllUsedSegments(datasourceToSummary); } else { retrieveUsedSegmentIds(datasourceToSummary); - updateSegmentIdsInCache(datasourceToSummary, syncStartTime); + updateSegmentIdsInCache(datasourceToSummary, syncStartTime.minus(SYNC_BUFFER_DURATION)); retrieveUsedSegmentPayloads(datasourceToSummary); } updateUsedSegmentPayloadsInCache(datasourceToSummary); retrieveAllPendingSegments(datasourceToSummary); - updatePendingSegmentsInCache(datasourceToSummary, syncStartTime); + updatePendingSegmentsInCache(datasourceToSummary, syncStartTime.minus(SYNC_BUFFER_DURATION)); if (useSchemaCache) { retrieveAndResetUsedSegmentSchemas(datasourceToSummary); diff --git a/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java b/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java index 2a5940b04951..02546bed20dc 100644 --- a/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java +++ b/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java @@ -579,7 +579,7 @@ public void testSync_removesUnusedSegment_ifCacheHasOlderEntry() TestDataSource.WIKI, wikiCache -> wikiCache.markSegmentAsUnused( unpersistedSegmentPlus.getDataSegment().getId(), - now.minusMinutes(1) + now.minusHours(1) ) ); From 1a07db58bb84d12501d76e03c59e81dfc8268738 Mon Sep 17 00:00:00 2001 From: Kashif Faraz Date: Mon, 19 May 2025 19:57:23 +0530 Subject: [PATCH 17/18] Reduce buffer window to 10s --- .../segment/cache/HeapMemorySegmentMetadataCache.java | 6 +++++- .../segment/cache/HeapMemorySegmentMetadataCacheTest.java | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java b/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java index 0c3dd4c2f786..2a1cf50133e4 100644 --- a/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java +++ b/server/src/main/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCache.java @@ -116,8 +116,12 @@ public class HeapMemorySegmentMetadataCache implements SegmentMetadataCache * This is primarily needed to handle a race condition between insert and sync * where an entry with an updated time just before the sync start is added to * the cache just after the sync has started. + *

    + * This means that non-leader Overlord and all Coordinators will continue to + * consider a segment as used if it was marked as unused within the buffer period + * of a previous update (e.g. segment created, marked used or schema info updated). */ - private static final Duration SYNC_BUFFER_DURATION = Duration.standardMinutes(10); + private static final Duration SYNC_BUFFER_DURATION = Duration.standardSeconds(10); private enum CacheState { diff --git a/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java b/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java index 02546bed20dc..2a5940b04951 100644 --- a/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java +++ b/server/src/test/java/org/apache/druid/metadata/segment/cache/HeapMemorySegmentMetadataCacheTest.java @@ -579,7 +579,7 @@ public void testSync_removesUnusedSegment_ifCacheHasOlderEntry() TestDataSource.WIKI, wikiCache -> wikiCache.markSegmentAsUnused( unpersistedSegmentPlus.getDataSegment().getId(), - now.minusHours(1) + now.minusMinutes(1) ) ); From 3ae591bca763bc6709e9626f2ad167549dfd56aa Mon Sep 17 00:00:00 2001 From: Kashif Faraz Date: Mon, 19 May 2025 20:58:47 +0530 Subject: [PATCH 18/18] Fix tests --- .../IndexerSQLMetadataStorageCoordinatorTest.java | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java b/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java index 3042f53124e5..9a1b7ce63edf 100644 --- a/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java +++ b/server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java @@ -997,7 +997,7 @@ public void testRetrieveUsedSegmentForId() public void testRetrieveSegmentForId() { coordinator.commitSegments(Set.of(defaultSegment), null); - markAllSegmentsUnused(ImmutableSet.of(defaultSegment), DateTimes.nowUtc()); + coordinator.markSegmentAsUnused(defaultSegment.getId()); Assert.assertEquals( defaultSegment, coordinator.retrieveSegmentForId(defaultSegment.getId()) @@ -2613,8 +2613,7 @@ public void testAllocatePendingSegmentAfterDroppingExistingSegment() Assert.assertEquals(1, identifier3.getShardSpec().getNumCorePartitions()); // now drop the used segment previously loaded: - markAllSegmentsUnused(ImmutableSet.of(segment), DateTimes.nowUtc()); - refreshCache(); + coordinator.markSegmentAsUnused(segment.getId()); // and final load, this reproduces an issue that could happen with multiple streaming appends, // followed by a reindex, followed by a drop, and more streaming data coming in for same interval @@ -2784,8 +2783,7 @@ public void testAnotherAllocatePendingSegmentAfterRevertingCompaction() // 5) reverted compaction (by marking B_0 as unused) // Revert compaction a manual metadata update which is basically the following two steps: - markAllSegmentsUnused(ImmutableSet.of(compactedSegment), DateTimes.nowUtc()); // <- drop compacted segment - refreshCache(); + coordinator.markSegmentAsUnused(compactedSegment.getId()); // pending: version = A, id = 0,1,2 // version = B, id = 1 // @@ -3638,8 +3636,7 @@ public void testTimelineWith1CorePartitionTombstone() Assert.assertTrue(coordinator.commitSegments(dataSegments, new SegmentSchemaMapping(CentralizedDatasourceSchemaConfig.SCHEMA_VERSION)).containsAll(dataSegments)); // Mark the tombstone as unused - markAllSegmentsUnused(tombstones, DateTimes.nowUtc()); - refreshCache(); + coordinator.markSegmentAsUnused(tombstoneSegment.getId()); final Collection allUsedSegments = coordinator.retrieveAllUsedSegments( TestDataSource.WIKI,