From 0917dcd5d30187b368770804c45a6b1ec992b471 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Thu, 18 Jul 2019 14:22:18 -0700 Subject: [PATCH 1/4] fix npe with sql metadata manager polling and empty database --- .../metadata/SQLMetadataSegmentManager.java | 24 ++++++++++--------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/server/src/main/java/org/apache/druid/metadata/SQLMetadataSegmentManager.java b/server/src/main/java/org/apache/druid/metadata/SQLMetadataSegmentManager.java index db1d19e49ae9..25bdab081c41 100644 --- a/server/src/main/java/org/apache/druid/metadata/SQLMetadataSegmentManager.java +++ b/server/src/main/java/org/apache/druid/metadata/SQLMetadataSegmentManager.java @@ -65,6 +65,7 @@ import java.sql.SQLException; import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; import java.util.Iterator; import java.util.List; import java.util.Map; @@ -950,14 +951,7 @@ public DataSegment map(int index, ResultSet r, StatementContext ctx) throws SQLE } ); - if (segments == null || segments.isEmpty()) { - log.info("No segments found in the database!"); - return; - } - log.info("Polled and found %,d segments in the database", segments.size()); - - ImmutableMap dataSourceProperties = createDefaultDataSourceProperties(); // dataSourcesSnapshot is updated only here and the DataSourcesSnapshot object is immutable. If data sources or // segments are marked as used or unused directly (via markAs...() methods in MetadataSegmentManager), the @@ -967,10 +961,18 @@ public DataSegment map(int index, ResultSet r, StatementContext ctx) throws SQLE // segment mark calls in rapid succession. So the snapshot update is not done outside of database poll at this time. // Updates outside of database polls were primarily for the user experience, so users would immediately see the // effect of a segment mark call reflected in MetadataResource API calls. - dataSourcesSnapshot = DataSourcesSnapshot.fromUsedSegments( - Iterables.filter(segments, Objects::nonNull), // Filter corrupted entries (see above in this method). - dataSourceProperties - ); + + ImmutableMap dataSourceProperties = createDefaultDataSourceProperties(); + if (segments == null || segments.isEmpty()) { + log.info("No segments found in the database!"); + dataSourcesSnapshot = DataSourcesSnapshot.fromUsedSegments(Collections.emptyList(), dataSourceProperties); + } else { + log.info("Polled and found %,d segments in the database", segments.size()); + dataSourcesSnapshot = DataSourcesSnapshot.fromUsedSegments( + Iterables.filter(segments, Objects::nonNull), // Filter corrupted entries (see above in this method). + dataSourceProperties + ); + } } private static ImmutableMap createDefaultDataSourceProperties() From f1765038b338821e9d06f64ade5ea8907d23f071 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Fri, 19 Jul 2019 02:43:39 -0700 Subject: [PATCH 2/4] treat null segments separately --- .../metadata/SQLMetadataSegmentManager.java | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/server/src/main/java/org/apache/druid/metadata/SQLMetadataSegmentManager.java b/server/src/main/java/org/apache/druid/metadata/SQLMetadataSegmentManager.java index 25bdab081c41..da1085d658ca 100644 --- a/server/src/main/java/org/apache/druid/metadata/SQLMetadataSegmentManager.java +++ b/server/src/main/java/org/apache/druid/metadata/SQLMetadataSegmentManager.java @@ -65,7 +65,6 @@ import java.sql.SQLException; import java.util.ArrayList; import java.util.Collection; -import java.util.Collections; import java.util.Iterator; import java.util.List; import java.util.Map; @@ -951,7 +950,10 @@ public DataSegment map(int index, ResultSet r, StatementContext ctx) throws SQLE } ); - + if (segments == null) { + log.wtf("Observed 'null' when polling segments from the db, aborting snapshot update."); + return; + } // dataSourcesSnapshot is updated only here and the DataSourcesSnapshot object is immutable. If data sources or // segments are marked as used or unused directly (via markAs...() methods in MetadataSegmentManager), the @@ -963,16 +965,15 @@ public DataSegment map(int index, ResultSet r, StatementContext ctx) throws SQLE // effect of a segment mark call reflected in MetadataResource API calls. ImmutableMap dataSourceProperties = createDefaultDataSourceProperties(); - if (segments == null || segments.isEmpty()) { + if (segments.isEmpty()) { log.info("No segments found in the database!"); - dataSourcesSnapshot = DataSourcesSnapshot.fromUsedSegments(Collections.emptyList(), dataSourceProperties); } else { log.info("Polled and found %,d segments in the database", segments.size()); - dataSourcesSnapshot = DataSourcesSnapshot.fromUsedSegments( - Iterables.filter(segments, Objects::nonNull), // Filter corrupted entries (see above in this method). - dataSourceProperties - ); } + dataSourcesSnapshot = DataSourcesSnapshot.fromUsedSegments( + Iterables.filter(segments, Objects::nonNull), // Filter corrupted entries (see above in this method). + dataSourceProperties + ); } private static ImmutableMap createDefaultDataSourceProperties() From 5a4d43000b6c19f2bfb1bb04e1fd0024652d3cb5 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Fri, 19 Jul 2019 16:29:07 -0700 Subject: [PATCH 3/4] use preconditions check --- .../apache/druid/metadata/SQLMetadataSegmentManager.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/apache/druid/metadata/SQLMetadataSegmentManager.java b/server/src/main/java/org/apache/druid/metadata/SQLMetadataSegmentManager.java index da1085d658ca..2f37fd6ed719 100644 --- a/server/src/main/java/org/apache/druid/metadata/SQLMetadataSegmentManager.java +++ b/server/src/main/java/org/apache/druid/metadata/SQLMetadataSegmentManager.java @@ -950,10 +950,10 @@ public DataSegment map(int index, ResultSet r, StatementContext ctx) throws SQLE } ); - if (segments == null) { - log.wtf("Observed 'null' when polling segments from the db, aborting snapshot update."); - return; - } + Preconditions.checkNotNull( + segments, + "Unexpected 'null' when polling segments from the db, aborting snapshot update." + ); // dataSourcesSnapshot is updated only here and the DataSourcesSnapshot object is immutable. If data sources or // segments are marked as used or unused directly (via markAs...() methods in MetadataSegmentManager), the From 67775c2df38372308e51e93a7e0421af97988b02 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Fri, 19 Jul 2019 17:50:53 -0700 Subject: [PATCH 4/4] add test --- .../SQLMetadataSegmentManagerEmptyTest.java | 113 ++++++++++++++++++ 1 file changed, 113 insertions(+) create mode 100644 server/src/test/java/org/apache/druid/metadata/SQLMetadataSegmentManagerEmptyTest.java diff --git a/server/src/test/java/org/apache/druid/metadata/SQLMetadataSegmentManagerEmptyTest.java b/server/src/test/java/org/apache/druid/metadata/SQLMetadataSegmentManagerEmptyTest.java new file mode 100644 index 000000000000..1e3f5d8e25bc --- /dev/null +++ b/server/src/test/java/org/apache/druid/metadata/SQLMetadataSegmentManagerEmptyTest.java @@ -0,0 +1,113 @@ +/* + * 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; + +import com.fasterxml.jackson.databind.ObjectMapper; +import com.google.common.base.Suppliers; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import org.apache.druid.client.ImmutableDruidDataSource; +import org.apache.druid.segment.TestHelper; +import org.joda.time.Period; +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; + +import java.util.stream.Collectors; + + +/** + * Like {@link SQLMetadataRuleManagerTest} except with no segments to make sure it behaves when it's empty + */ +public class SQLMetadataSegmentManagerEmptyTest +{ + + @Rule + public final TestDerbyConnector.DerbyConnectorRule derbyConnectorRule = new TestDerbyConnector.DerbyConnectorRule(); + + private SQLMetadataSegmentManager sqlSegmentsMetadata; + private final ObjectMapper jsonMapper = TestHelper.makeJsonMapper(); + + @Before + public void setUp() throws Exception + { + TestDerbyConnector connector = derbyConnectorRule.getConnector(); + MetadataSegmentManagerConfig config = new MetadataSegmentManagerConfig(); + config.setPollDuration(Period.seconds(1)); + sqlSegmentsMetadata = new SQLMetadataSegmentManager( + jsonMapper, + Suppliers.ofInstance(config), + derbyConnectorRule.metadataTablesConfigSupplier(), + connector + ); + sqlSegmentsMetadata.start(); + + connector.createSegmentTable(); + } + + @After + public void teardown() + { + if (sqlSegmentsMetadata.isPollingDatabasePeriodically()) { + sqlSegmentsMetadata.stopPollingDatabasePeriodically(); + } + sqlSegmentsMetadata.stop(); + } + + @Test + public void testPollEmpty() + { + sqlSegmentsMetadata.startPollingDatabasePeriodically(); + sqlSegmentsMetadata.poll(); + Assert.assertTrue(sqlSegmentsMetadata.isPollingDatabasePeriodically()); + Assert.assertEquals( + ImmutableList.of(), + sqlSegmentsMetadata.retrieveAllDataSourceNames() + ); + Assert.assertEquals( + ImmutableList.of(), + sqlSegmentsMetadata + .getImmutableDataSourcesWithAllUsedSegments() + .stream() + .map(ImmutableDruidDataSource::getName) + .collect(Collectors.toList()) + ); + Assert.assertEquals( + null, + sqlSegmentsMetadata.getImmutableDataSourceWithUsedSegments("wikipedia") + ); + Assert.assertEquals( + ImmutableSet.of(), + ImmutableSet.copyOf(sqlSegmentsMetadata.iterateAllUsedSegments()) + ); + } + + @Test + public void testStopAndStart() + { + // Simulate successive losing and getting the coordinator leadership + sqlSegmentsMetadata.startPollingDatabasePeriodically(); + sqlSegmentsMetadata.stopPollingDatabasePeriodically(); + sqlSegmentsMetadata.startPollingDatabasePeriodically(); + sqlSegmentsMetadata.stopPollingDatabasePeriodically(); + } +}