Fix num_rows in sys.segments by surekhasaharan · Pull Request #6888 · apache/druid

surekhasaharan · 2019-01-20T07:05:39Z

num_rows column of sys.segments was incorrectly reported for some case. It happened because the segmentMetadataInfo in DruidSchema was overwritten in addSegment when the segment had more than one replica. This PR fixes:

segmentMetadataInfo update in DruidSchema
Add numRows to SegmentMetadataHolder builder's constructor, so it's not overwritten
Rename setSegmentSignature to setSegmentMetadataHolder
Replace Map<String, Set<String>> with Set<String> for num_replica

* Fix segmentMetadataInfo update in DruidSchema * Add numRows to SegmentMetadataHolder builder's constructor, so it's not overwritten * Rename SegSegmentSignature to setSegmentMetadataHolder and fix it so nested map is appended instead of recreated * Replace Map<String, Set<String>> segmentServerMap with Set<String> for num_replica

jihoonson · 2019-02-05T23:41:56Z

@surekhasaharan would you please fix the conflicts?

jihoonson · 2019-02-07T00:24:02Z

  // DataSource -> Segment -> SegmentMetadataHolder(contains RowSignature) for that segment.
  // Use TreeMap for segments so they are merged in deterministic order, from older to newer.
-  // This data structure need to be accessed in a thread-safe way since SystemSchema accesses it
+  // This data structure need to be accessed in a thread-safe way via lock Object


I think it's still better to say why the concurrency issue happens too.

Also please update https://github.com/apache/incubator-druid/blob/master/sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidSchema.java#L110.

updated comments

jihoonson · 2019-02-07T00:32:18Z

+  {
+    Map<DataSegment, SegmentMetadataHolder> segmentsMetadata = schema.getSegmentMetadata();
+    Set<DataSegment> segments = segmentsMetadata.keySet();
+    Assert.assertEquals(segments.size(), 3);


The arguments have an order. The expected value should be the first argument.

jihoonson · 2019-02-07T00:38:15Z

+    segmentsMetadata = schema.getSegmentMetadata();
+    existingSegment = segments.stream().findFirst().orElse(null);
+    final SegmentMetadataHolder currentHolder = segmentsMetadata.get(existingSegment);
+    Assert.assertEquals(updatedHolder.getNumRows(), currentHolder.getNumRows());


Please check other fields as well.

this test was for numRows, but sure can add other fields as well.

I think this kind of unit tests is to check the an entire state of a snapshot rather than a particular value of the snapshot. There's no reason to check individual values in different tests under the same situation.

jihoonson · 2019-02-07T00:39:14Z

  }

+  @Test
+  public void testSegmentMetadataHolderNumRows()


Would you please add a comment about what this method is testing? It's not intuitive from the code.

added comment

jihoonson · 2019-02-07T00:40:02Z

+    Set<DataSegment> segments = segmentsMetadata.keySet();
+    Assert.assertEquals(segments.size(), 3);
+    DataSegment existingSegment = segments.stream().findFirst().orElse(null);
+    Assert.assertFalse(existingSegment == null);


Assert.assertNotNull(existingSegment)

jihoonson · 2019-02-07T00:43:18Z

+    ImmutableDruidServer server = null;
+    for (ImmutableDruidServer druidServer : druidServers) {
+      for (DataSegment segment : druidServer.getSegments()) {
+        if (segment == existingSegment) {


It looks that this is to find a druidServer holding existingSegment. Why not

druidServers.stream() .flatMap(druidServer -> druidServer.getSegments().stream()) .filter(segment -> segment.equals(existingSegment)) .findAny() .orElse(null)

? You also may want to make existingSegments final.

Also, I don't think you're comparing these segments with == for performance reason. Please use equals() instead.

changed to use streams api instead of nested loops

jihoonson · 2019-02-07T00:56:12Z

        2L, //partition_num
        1L, //num_replicas
-        3L, //numRows
+        2L, //numRows


Would you please explain what this change is for?

this change is because I created another ROWS3 and index3 above, such that now (segment3, index3) is added to walker, to make the test code more intuitive.

justinborromeo · 2019-02-07T18:47:57Z


-  private void addSegment(final DruidServerMetadata server, final DataSegment segment)
+  @VisibleForTesting
+  protected void addSegment(final DruidServerMetadata server, final DataSegment segment)


Would this be better as package private instead of protected to minimize scope? Since there's no subclasses in other packages, is there a need to be protected?

ah yeah, no need for this and others to be protected, can be changed to default package access.

justinborromeo · 2019-02-07T18:51:00Z


-  private void setSegmentSignature(final DataSegment segment, final SegmentMetadataHolder segmentMetadataHolder)
+  @VisibleForTesting
+  protected void setSegmentMetadataHolder(final DataSegment segment, final SegmentMetadataHolder segmentMetadataHolder)


Could this also be package-private?

justinborromeo · 2019-02-07T19:02:08Z

+  public long getNumReplicas()
  {
-    return segmentServerMap.get(segmentId).size();
+    return segmentServers.size();


Why do we return long in this method when Set#size() returns an int? Might not be worth changing because it might blow up other stuff.

It used to be int, and then got changed to long at some point. The reason is because we support nulls for long here, but not for ints ? Not very sure...but will likely leave it to be long here.

Sounds good

jihoonson

@surekhasaharan thanks for the quick fix. I left some trivial comments.

jihoonson · 2019-02-08T21:21:53Z

  // Use TreeMap for segments so they are merged in deterministic order, from older to newer.
-  // This data structure need to be accessed in a thread-safe way since SystemSchema accesses it
+  // This data structure need to be accessed in a thread-safe way via lock Object since segments can be added,
+  //removed, refreshed or accessed asynchronously


nit: please add a space after //.

ok, yeah will add at all the places

jihoonson · 2019-02-08T21:25:17Z

+    segmentsMetadata = schema.getSegmentMetadata();
+    existingSegment = segments.stream().findFirst().orElse(null);
+    final SegmentMetadataHolder currentHolder = segmentsMetadata.get(existingSegment);
+    Assert.assertEquals(updatedHolder.getNumRows(), currentHolder.getNumRows());


I think this kind of unit tests is to check the an entire state of a snapshot rather than a particular value of the snapshot. There's no reason to check individual values in different tests under the same situation.

jihoonson · 2019-02-08T21:25:35Z

        2L, //partition_num
        1L, //num_replicas
-        3L, //numRows
+        2L, //numRows


jihoonson · 2019-02-08T21:29:42Z

+                                                .orElse(null);
+    Assert.assertNotNull(existingSegment);
+    final SegmentMetadataHolder existingHolder = segmentsMetadata.get(existingSegment);
+    //update SegmentMetadataHolder of existingSegment with numRows=5


nit: please make the style of comments consistent. I think it's better to add a space after // because most of comments do.

Same for other comments.

gianm · 2019-01-31T20:20:05Z

  private static final int MAX_SEGMENTS_PER_QUERY = 15000;
  private static final long IS_PUBLISHED = 0;
  private static final long IS_AVAILABLE = 1;
+  private static final long NUM_ROWS = 0;


These three should really be called DEFAULT_IS_PUBLISHED, DEFAULT_IS_AVAILABLE, and DEFAULT_NUM_ROWS.

ok, renamed

gianm · 2019-01-31T20:20:54Z

  // Use TreeMap for segments so they are merged in deterministic order, from older to newer.
-  // This data structure need to be accessed in a thread-safe way since SystemSchema accesses it
+  // This data structure need to be accessed in a thread-safe way via lock Object
  private final Map<String, TreeMap<DataSegment, SegmentMetadataHolder>> segmentMetadataInfo = new HashMap<>();


Instead of this comment, you could add @GuardedBy("lock"). It sends the same message, and could also help with automated bug detection.

good suggestion, thanks.

jihoonson

LGTM after CI. @surekhasaharan thanks!

* Fix the bug with num_rows in sys.segments * Fix segmentMetadataInfo update in DruidSchema * Add numRows to SegmentMetadataHolder builder's constructor, so it's not overwritten * Rename SegSegmentSignature to setSegmentMetadataHolder and fix it so nested map is appended instead of recreated * Replace Map<String, Set<String>> segmentServerMap with Set<String> for num_replica * Remove unnecessary code and update test * Add unit test for num_rows * PR comments * change access modifier to default package level * minor changes to comments * PR comments

fjy added this to the 0.14.0 milestone Jan 20, 2019

fjy added the Bug label Jan 20, 2019

Surekha Saharan added 6 commits January 21, 2019 15:41

Merge branch 'master' of github.com:druid-io/druid into fix-num-rows

24fbe1c

Merge branch 'master' of github.com:druid-io/druid into fix-num-rows

ebeff85

Remove unnecessary code and update test

97721d1

Merge branch 'master' of github.com:druid-io/druid into fix-num-rows

22d8304

Add unit test for num_rows

b27a3d4

Merge branch 'master' of github.com:druid-io/druid into fix-num-rows

68ec9c7

gianm assigned jihoonson Feb 5, 2019

Surekha Saharan added 2 commits February 5, 2019 15:49

Merge branch 'master' of github.com:druid-io/druid into fix-num-rows

ae66255

Merge branch 'master' of github.com:druid-io/druid into fix-num-rows

1a1ccff

jihoonson reviewed Feb 7, 2019

View reviewed changes

Surekha Saharan added 2 commits February 7, 2019 10:16

PR comments

57778a1

Merge branch 'master' of github.com:druid-io/druid into fix-num-rows

24085bc

justinborromeo reviewed Feb 7, 2019

View reviewed changes

Surekha Saharan added 2 commits February 7, 2019 23:05

change access modifier to default package level

8eb2ac0

Merge branch 'master' of github.com:druid-io/druid into fix-num-rows

afbf83f

jihoonson reviewed Feb 8, 2019

View reviewed changes

Surekha Saharan added 2 commits February 8, 2019 15:53

minor changes to comments

e29bedb

Merge branch 'master' of github.com:druid-io/druid into fix-num-rows

c2640e6

gianm reviewed Feb 10, 2019

View reviewed changes

Surekha Saharan added 2 commits February 10, 2019 15:29

PR comments

11b84dd

Merge branch 'master' of github.com:druid-io/druid into fix-num-rows

5720a96

jihoonson approved these changes Feb 11, 2019

View reviewed changes

jihoonson merged commit 02ef14f into apache:master Feb 12, 2019

surekhasaharan mentioned this pull request Feb 12, 2019

Backport 6888 to 0.14.0 incubating #7053

Closed

surekhasaharan mentioned this pull request Feb 12, 2019

[Backport] Fix num_rows in sys.segments (#6888) #7054

Merged

surekhasaharan deleted the fix-num-rows branch February 20, 2019 18:48

Conversation

surekhasaharan commented Jan 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jihoonson commented Feb 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinborromeo Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jihoonson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

surekhasaharan commented Jan 20, 2019 •

edited

Loading

justinborromeo Feb 7, 2019 •

edited

Loading