From 234afd39d9297225c85973eec6f4fe03cffb4b0a Mon Sep 17 00:00:00 2001 From: Surekha Saharan Date: Wed, 9 Jan 2019 17:50:58 -0800 Subject: [PATCH 1/5] Add null checks in DruidSchema --- .../druid/sql/calcite/schema/DruidSchema.java | 34 +++++++++++++------ 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidSchema.java b/sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidSchema.java index 87d97ef29e9c..e324fa2a21b9 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidSchema.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidSchema.java @@ -469,17 +469,29 @@ private Set refreshSegmentsForDataSource( final RowSignature rowSignature = analysisToRowSignature(analysis); log.debug("Segment[%s] has signature[%s].", segment.getIdentifier(), rowSignature); final Map dataSourceSegments = segmentMetadataInfo.get(segment.getDataSource()); - SegmentMetadataHolder holder = dataSourceSegments.get(segment); - SegmentMetadataHolder updatedHolder = new SegmentMetadataHolder.Builder( - holder.getSegmentId(), - holder.isPublished(), - holder.isAvailable(), - holder.isRealtime(), - holder.getNumReplicas() - ).withRowSignature(rowSignature).withNumRows(analysis.getNumRows()).build(); - dataSourceSegments.put(segment, updatedHolder); - setSegmentSignature(segment, updatedHolder); - retVal.add(segment); + if (dataSourceSegments == null) { + log.warn("No segment map found with datasource[%s], skipping refresh", segment.getDataSource()); + } else { + SegmentMetadataHolder holder = dataSourceSegments.get(segment); + if (holder == null) { + log.warn( + "No segment[%s] found with datasource[%s], skipping refresh", + segment.getIdentifier(), + segment.getDataSource() + ); + } else { + SegmentMetadataHolder updatedHolder = new SegmentMetadataHolder.Builder( + holder.getSegmentId(), + holder.isPublished(), + holder.isAvailable(), + holder.isRealtime(), + holder.getNumReplicas() + ).withRowSignature(rowSignature).withNumRows(analysis.getNumRows()).build(); + dataSourceSegments.put(segment, updatedHolder); + setSegmentSignature(segment, updatedHolder); + retVal.add(segment); + } + } } } From 0d45ec5ffa969471ddb9c6806920b63ebd6b9c67 Mon Sep 17 00:00:00 2001 From: Surekha Saharan Date: Thu, 24 Jan 2019 17:22:27 -0800 Subject: [PATCH 2/5] Add unit tests --- .../druid/sql/calcite/schema/DruidSchema.java | 11 +++---- .../sql/calcite/schema/DruidSchemaTest.java | 33 +++++++++++++++++++ 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidSchema.java b/sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidSchema.java index 4275981b6ae9..2dd6f62bcddb 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidSchema.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidSchema.java @@ -402,7 +402,7 @@ private void addSegment(final DruidServerMetadata server, final DataSegment segm } } - private void removeSegment(final DataSegment segment) + protected void removeSegment(final DataSegment segment) { synchronized (lock) { log.debug("Segment[%s] is gone.", segment.getId()); @@ -450,7 +450,7 @@ private void removeServerSegment(final DruidServerMetadata server, final DataSeg * Attempt to refresh "segmentSignatures" for a set of segments. Returns the set of segments actually refreshed, * which may be a subset of the asked-for set. */ - private Set refreshSegments(final Set segments) throws IOException + protected Set refreshSegments(final Set segments) throws IOException { final Set retVal = new HashSet<>(); @@ -512,9 +512,8 @@ private Set refreshSegmentsForDataSource(final String dataSource, f SegmentMetadataHolder holder = dataSourceSegments.get(segment); if (holder == null) { log.warn( - "No segment[%s] found with datasource[%s], skipping refresh", - segment.getId(), - segment.getDataSource() + "No segment[%s] found, skipping refresh", + segment.getId() ); } else { SegmentMetadataHolder updatedHolder = SegmentMetadataHolder @@ -640,7 +639,7 @@ private static RowSignature analysisToRowSignature(final SegmentAnalysis analysi return rowSignatureBuilder.build(); } - public Map getSegmentMetadata() + Map getSegmentMetadata() { final Map segmentMetadata = new HashMap<>(); synchronized (lock) { diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/schema/DruidSchemaTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/schema/DruidSchemaTest.java index 1b4e00819bb6..d3c62ad0ceec 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/schema/DruidSchemaTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/schema/DruidSchemaTest.java @@ -63,6 +63,7 @@ import java.io.IOException; import java.util.List; import java.util.Map; +import java.util.Set; public class DruidSchemaTest extends CalciteTestBase { @@ -237,4 +238,36 @@ public void testGetTableMapFoo2() Assert.assertEquals("m1", fields.get(2).getName()); Assert.assertEquals(SqlTypeName.BIGINT, fields.get(2).getType().getSqlTypeName()); } + + @Test + public void testNullDatasource() throws IOException + { + Map segmentsMetadata = schema.getSegmentMetadata(); + Set segments = segmentsMetadata.keySet(); + Assert.assertEquals(segments.size(), 3); + final DataSegment segmentToRemove = segments.stream().findFirst().orElse(null); + Assert.assertFalse(segmentToRemove == null); + schema.removeSegment(segmentToRemove); + schema.refreshSegments(segments); // can cause NPE without dataSourceSegments null check in DruidSchema#refreshSegmentsForDataSource + segmentsMetadata = schema.getSegmentMetadata(); + segments = segmentsMetadata.keySet(); + Assert.assertEquals(segments.size(), 2); + } + + @Test + public void testNullSegmentMetadataHolder() throws IOException + { + Map segmentsMetadata = schema.getSegmentMetadata(); + Set segments = segmentsMetadata.keySet(); + Assert.assertEquals(segments.size(), 3); + //remove the 2nd segment with datasource foo + final DataSegment segmentToRemove = segments.stream().skip(1).findFirst().orElse(null); + Assert.assertFalse(segmentToRemove == null); + schema.removeSegment(segmentToRemove); + schema.refreshSegments(segments); // can cause NPE without holder null check in SegmentMetadataHolder#from + segmentsMetadata = schema.getSegmentMetadata(); + segments = segmentsMetadata.keySet(); + Assert.assertEquals(segments.size(), 2); + } + } From 00c1b37ba3929680535cf610b8858e35481beb67 Mon Sep 17 00:00:00 2001 From: Surekha Saharan Date: Fri, 25 Jan 2019 20:59:30 -0800 Subject: [PATCH 3/5] Add VisibleForTesting annotation --- .../java/org/apache/druid/sql/calcite/schema/DruidSchema.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidSchema.java b/sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidSchema.java index 2dd6f62bcddb..2929229b4c59 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidSchema.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidSchema.java @@ -19,6 +19,7 @@ package org.apache.druid.sql.calcite.schema; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.base.Predicates; import com.google.common.collect.FluentIterable; @@ -402,6 +403,7 @@ private void addSegment(final DruidServerMetadata server, final DataSegment segm } } + @VisibleForTesting protected void removeSegment(final DataSegment segment) { synchronized (lock) { @@ -450,6 +452,7 @@ private void removeServerSegment(final DruidServerMetadata server, final DataSeg * Attempt to refresh "segmentSignatures" for a set of segments. Returns the set of segments actually refreshed, * which may be a subset of the asked-for set. */ + @VisibleForTesting protected Set refreshSegments(final Set segments) throws IOException { final Set retVal = new HashSet<>(); From c6f0a69e9ea0005d255787c17501ea5db43cd705 Mon Sep 17 00:00:00 2001 From: Surekha Saharan Date: Thu, 31 Jan 2019 10:47:07 -0800 Subject: [PATCH 4/5] PR comments --- .../sql/calcite/schema/DruidSchemaTest.java | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/schema/DruidSchemaTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/schema/DruidSchemaTest.java index d3c62ad0ceec..c1c4cb243cc5 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/schema/DruidSchemaTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/schema/DruidSchemaTest.java @@ -63,6 +63,7 @@ import java.io.IOException; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Set; public class DruidSchemaTest extends CalciteTestBase @@ -242,31 +243,33 @@ public void testGetTableMapFoo2() @Test public void testNullDatasource() throws IOException { - Map segmentsMetadata = schema.getSegmentMetadata(); - Set segments = segmentsMetadata.keySet(); + Map segmentMetadatas = schema.getSegmentMetadata(); + Set segments = segmentMetadatas.keySet(); Assert.assertEquals(segments.size(), 3); - final DataSegment segmentToRemove = segments.stream().findFirst().orElse(null); + // segments contains two segments with datasource "foo" and one with datasource "foo2" + // let's remove the only segment with datasource "foo2" + final DataSegment segmentToRemove = segments.stream().filter(segment -> segment.getDataSource().equals("foo2")).findFirst().orElse(null); Assert.assertFalse(segmentToRemove == null); schema.removeSegment(segmentToRemove); schema.refreshSegments(segments); // can cause NPE without dataSourceSegments null check in DruidSchema#refreshSegmentsForDataSource - segmentsMetadata = schema.getSegmentMetadata(); - segments = segmentsMetadata.keySet(); + segmentMetadatas = schema.getSegmentMetadata(); + segments = segmentMetadatas.keySet(); Assert.assertEquals(segments.size(), 2); } @Test public void testNullSegmentMetadataHolder() throws IOException { - Map segmentsMetadata = schema.getSegmentMetadata(); - Set segments = segmentsMetadata.keySet(); + Map segmentMetadatas = schema.getSegmentMetadata(); + Set segments = segmentMetadatas.keySet(); Assert.assertEquals(segments.size(), 3); - //remove the 2nd segment with datasource foo - final DataSegment segmentToRemove = segments.stream().skip(1).findFirst().orElse(null); + //remove one of the segments with datasource "foo" + final DataSegment segmentToRemove = segments.stream().filter(segment -> segment.getDataSource().equals("foo")).findFirst().orElse(null); Assert.assertFalse(segmentToRemove == null); schema.removeSegment(segmentToRemove); schema.refreshSegments(segments); // can cause NPE without holder null check in SegmentMetadataHolder#from - segmentsMetadata = schema.getSegmentMetadata(); - segments = segmentsMetadata.keySet(); + segmentMetadatas = schema.getSegmentMetadata(); + segments = segmentMetadatas.keySet(); Assert.assertEquals(segments.size(), 2); } From 0fa68610eb76b427f92d3b52c5ed0f7ca91a1178 Mon Sep 17 00:00:00 2001 From: Surekha Saharan Date: Thu, 31 Jan 2019 12:05:58 -0800 Subject: [PATCH 5/5] unused import --- .../org/apache/druid/sql/calcite/schema/DruidSchemaTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/schema/DruidSchemaTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/schema/DruidSchemaTest.java index c1c4cb243cc5..30b99e59ab44 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/schema/DruidSchemaTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/schema/DruidSchemaTest.java @@ -63,7 +63,6 @@ import java.io.IOException; import java.util.List; import java.util.Map; -import java.util.Optional; import java.util.Set; public class DruidSchemaTest extends CalciteTestBase