Fix ambiguity about IndexerSQLMetadataStorageCoordinator.getUsedSegmentsForInterval() returning only non-overshadowed or all used segments by leventov · Pull Request #8564 · apache/druid

leventov · 2019-09-20T14:41:10Z

A PR that helps with failing tests in #7306.

Description

Semantic changes, API changes, corrections

The specification (and the names) of IndexerMetadataStorageCoordinator.getUsedSegmentsForInterval() and getUsedSegmentsForIntervals() methods used to say nothing about whether it returns only visible used segments in the specified interval(s) or all used segments. The implementation used to return only visible segments. I added Segments visibility parameter to these methods, where Segments is a enum with values ONLY_VISIBLE and INCLUDING_OVERSHADOWED, to force users of these methods to always make an explicit choice. I'm not sure all existing users of this API actually want only visible segments. These include:
- MaterializedViewSupervisor. FYI @sekingme, @zhangxinyu1
- HadoopIngestionSpec. FYI @nishantmonu51
- ActionBasedUsedSegmentChecker. FYI @jihoonson, @gianm
- SegmentAllocateAction. FYI @jihoonson, @gianm
- CompactionTask. FYI @jihoonson
- MetadataResource: this is the place where I've changed the former behavior: /datasources/{dataSourceName}/segments endpoint now returns all used segments (including overshadowed) on the specified intervals, rather than only visible ones. This is a potentially Incompatible change, so it warrans Design Review and a mention in Release Notes.
getUsedSegmentsForInterval() and getUsedSegmentsForIntervals() now also return a Collection instead of a List to emphasize that the order of the returned segments is unspecified (and also to avoid some unnecesssary copying between collections in the implementation).
Renamed UsedSegmentsLister to UsedSegmentsRetriever. The immediate reason for this rename is that this command doesn't return a List anymore (now it returns a Collection). The new name also makes more apparent the cost of this operation (an RPC call). Also, added Segments visibility parameter to this command to reflect the changes in IndexerMetadataStorageCoordinator's API. Note: UsedSegmentsLister is not Druid's public or extension API.
Renamed SegmentListUsedAction to RetrieveUsedSegmentsAction and SegmentListUnusedAction to RetrieveUnusedSegmentsAction. Also, added Segments visibility parameter to the first action. The JSON serialization names of the commands are not changed, for backward compatibility: it's still "segmentListUsed" and "segmentListUnused", respectively. Even after updating Druid to Jackson 2.9, these names may not be changed as with property names (see Rename poorly named properties and config options when updating to Jackson 2.9 #7152), because these names are specified via a different beast: JsonSubTypes.Type. The addition of Segments visibility parameter (including to the JSON serialization form) should be backward- and forward-compatible in Jackson. When absent, the parameter defaults to ONLY_VISIBLE (the former behavior).
MetadataSegmentManager.retrieveAllDataSourceNames() now returns a Set instead of a Collection.

Fixes in tests

A lot of changes primarily in KafkaIndexTaskTest and KinesisIndexTaskTest which relied on a specific order of results returned from IndexerMetadataStorageCoordinator.getUsedSegmentsForInterval() which was unspecified and changed in this PR.

Refactorings and new utility methods

Added JacksonUtils.readValue() to reduce boilerplate with catching IOException and wrapping it into a RuntimeException.
Added VersionedIntervalTimeline.findNonOvershadowedObjectsInInterval() which before appeared in multiple places in the codebase ad-hoc. This change also fixes locking duplicating segments in AbstractBatchIndexTask, which could have been a bug, or benign unnecessary behavior. FYI @jihoonson. (This is the reason why this PR is labeled Bug.)

Refactoring of tests

Extracted class abstract class SeekableStreamIndexTaskTestBase to reduce duplication between KafkaIndexTaskTest and KinesisIndexTaskTest. Note that this contradicts the advice to favor composition and "Fixture" classes over inheritance in tests which I brought up a few days ago in the mailing list. This is because with the current Druid's policy to not use static imports, usage of static methods would be way too mouthful. There may be a way to refactor these tests in object terms to create composable object fixtures. That would be better than inheritance, but I didn't explore this because this work would be too far outside of the scope of the PR. Anyway, I think at least reducing duplication via a common base class is a step in the right direction, both compared to not doing anything and leaving the duplicated code as is, and as an intermediate step towards object composition refactoring, if somebody endeavors to do it in the future.
Extracted VersionedIntervalTimelineSpecificDataTest from VersionedIntervalTimelineTest. Half of the methods in former VersionedIntervalTimelineTest required a different setUp() than the other half. I've split these halves between separate classes to avoid confusion. However, I've also added a common base class VersionedIntervalTimelineTestBase for them to avoid duplication. In this case, just allowing static imports from one class to another would make this unnecessary. I'll write about this in the mailing list.

Optimization

Don't create an intermediate List in PartitionHolder.iterator(), spliterator(), and stream() methods. See changes in PartitionHolder and OvershadowableManager classes.
Don't copy data into an intermediate List in the implementation of NewestSegmentFirstIterator and unnecessary sort in findSegmentsToCompact().

This PR has:

been self-reviewed.

…e() don't fetch abutting intervals; simplify getUsedSegmentsForIntervals()

nishantmonu51

👍

egor-ryashin

LGTM

jihoonson · 2019-09-24T12:01:47Z

-              })
-              .distinct()
-              .collect(Collectors.toList());
+          return new ArrayList<>(timeline.iterateAllObjects());


This can result in a different set of holders. lookup() can adjust the first and last holders so that they are aligned with the given interval.

It doesn't matter because the result of this method is a collection of objects, not holders.

…method; Propagate the decision about whether only visible segmetns or visible and overshadowed segments should be returned from IndexerMetadataStorageCoordinator's methods to the user logic; Rename SegmentListUsedAction to RetrieveUsedSegmentsAction, SegmetnListUnusedAction to RetrieveUnusedSegmentsAction, and UsedSegmentLister to UsedSegmentsRetriever

gianm · 2019-09-30T22:49:02Z

The checkstyle issue looks legitimate; I'm not sure about the strict compilation one.

…sonUtils.readValue() to reduce boilerplate code

…egmentsForInterval

…skTest and KafkaIndexTaskTest into SeekableStreamIndexTaskTestBase

…egmentsForInterval

leventov · 2019-10-11T11:44:46Z

A much larger issue (ambiguity of getUsedSegmentsForInterval()) surfaced when I investigated the test failures in the original "small refactoring" PR. This is now a completely different, much larger PR. See the updated description.

@egor-ryashin @nishantmonu51 @gianm @jihoonson.

…egmentsForInterval

leventov · 2019-10-29T10:58:59Z

Could somebody please review this PR?

jihoonson · 2019-10-29T16:50:45Z


 /**
+ * This enum is used a parameter for several methods in {@link VersionedIntervalTimeline}, specifying whether only
+ * complete partitions should be considered, or incomplete partitions as well.


Should this explain what "complete partition" means? Also it can link PartitionHolder.isComplete().

Since VersionedIntervalTimeline treats completion status mechanically rather than semantically (that is, the implementation of VersionedIntervalTimeline would be the same regardless of what isComplete() semantically means), I just added reference to the method. I think Javadoc should be added to isComplete() method to explain illustratively what does it mean, but I don't feel confident about it. I've also opened issue #8788 which is related.

jihoonson · 2019-10-29T17:38:58Z

+        .list();
+  }
+
+  private Query<Map<String, Object>> createUsedSegmentsSqlQueryForIntervals(


Would you please add a javadoc for this method?

jihoonson · 2019-10-29T17:50:16Z

+
+/**
+ * This enum is used as a parameter for several methods in {@link IndexerMetadataStorageCoordinator}, specifying whether
+ * only visible segments, or visible as well as overshadowed segments should be included in results.


Since this enum is used only in IndexerMetadataStorageCoordinator, it will be more accurate if it says all segments in the result are published segments.

I think this would be overspecification for the enum. Instead, I've added "published" adjective to descriptions of all related methods in IndexerMetadataStorageCoordinator.

jihoonson · 2019-10-29T17:52:28Z


  @Test
-  public void testSegmentListUsedAction()
+  public void testRetrieveUsedSegmentsAction()


Please change the name of this class as well.

Thanks, changed

jihoonson · 2019-10-29T17:56:31Z

+import java.util.Set;
+import java.util.stream.Collectors;
+
+public class SeekableStreamIndexTaskTestBase extends EasyMockSupport


I didn't check all tests in Kafka/KinesisIndexTaskTest, but just assume they are basically same with before except that common parts are extracted as this base class. Is this correct?

jihoonson · 2019-10-29T17:58:46Z

+   * @param interval   The interval for which all applicable and used datasources are requested. Start is inclusive,
+   *                   end is exclusive
+   * @param visibility Whether only visible or visible as well as overshadowed segments should be returned. The
+   *                   visibility is considered within the specified interval: that is, a segment which is globally


What does it mean by "globally visible"?

Reworded to "visible outside of the specified interval(s)"

It's still unclear to me what "visible outside of an interval" means. Does this imply that a segment can be overshadowed only when we want to find the most recent one among segments in the same interval?

I didn't understand your question but added a more detailed description of visibility to the Javadoc for Segments and linked from the methods. See this commmit.

jihoonson · 2019-10-29T18:00:53Z

    for (int i = 0; i < intervals.size(); i++) {
      sb.append(
-          StringUtils.format("(start <= ? AND %1$send%1$s >= ?)", connector.getQuoteString())
+          StringUtils.format("(start < ? AND %1$send%1$s > ?)", connector.getQuoteString())


Nice catch!

jihoonson · 2019-11-05T22:37:08Z

+ * the intervals in the series.
+ *
+ * If not specified otherwise, visibility (or overshadowness) should be assumed on the interval (-inf, +inf). This
+ * visibility may also be called "global" or "general" visibility.


This new doc looks nice. Just one question is, do we need the concept of "global visibility"? Seems like it's enough to say If not specified otherwise, visibility (or overshadowness) should be assumed on the interval (-inf, +inf)..

Ok, removed the last sentence.

…egmentsForInterval

jihoonson

LGTM. Thanks for quick update.

IndexerSQLMetadataStorageCoordinator.getTimelineForIntervalsWithHandl…

d5e3127

…e() don't fetch abutting intervals; simplify getUsedSegmentsForIntervals()

leventov added Refactoring Area - Metadata labels Sep 20, 2019

leventov requested a review from egor-ryashin September 20, 2019 14:41

nishantmonu51 approved these changes Sep 20, 2019

View reviewed changes

leventov added the WIP label Sep 20, 2019

egor-ryashin approved these changes Sep 23, 2019

View reviewed changes

jihoonson reviewed Sep 24, 2019

View reviewed changes

leventov added the Bug label Sep 27, 2019

leventov added 8 commits October 1, 2019 15:11

Fix tests

085d934

More fixes

1de4204

Add javadoc notes about returning Collection instead of Set. Add Jack…

b45ad10

…sonUtils.readValue() to reduce boilerplate code

Merge remote-tracking branch 'upstream/master' into refactor-getUsedS…

b4a67db

…egmentsForInterval

Fix KinesisIndexTaskTest, factor out common parts from KinesisIndexTa…

58482c9

…skTest and KafkaIndexTaskTest into SeekableStreamIndexTaskTestBase

More test fixes

697cb94

More test fixes

cd4a400

Merge remote-tracking branch 'upstream/master' into refactor-getUsedS…

f3543b2

…egmentsForInterval

leventov added Design Review Release Notes Incompatible labels Oct 11, 2019

Add a comment to VersionedIntervalTimelineTestBase

b1897ad

leventov removed the WIP label Oct 11, 2019

leventov added 4 commits October 21, 2019 11:05

Merge remote-tracking branch 'upstream/master' into refactor-getUsedS…

dc657c0

…egmentsForInterval

Fix tests

7a8a123

Set DataSegment.size(0) in more tests

bd8be52

Specify DataSegment.size(0) in more places in tests

178f083

leventov added 5 commits October 27, 2019 12:45

Fix more tests

998def8

Merge remote-tracking branch 'upstream/master' into refactor-getUsedS…

186d751

…egmentsForInterval

Fix DruidSchemaTest

91545c7

Set DataSegment's size in more tests and benchmarks

7137805

Fix HdfsDataSegmentPusherTest

3053f8a

jihoonson reviewed Oct 29, 2019

View reviewed changes

leventov added 4 commits October 30, 2019 17:41

Doc changes addressing comments

1c45ecd

Extended doc for visibility

2fc7c5b

Typo

448685b

Typo 2

023e96c

jihoonson reviewed Nov 5, 2019

View reviewed changes

leventov added 2 commits November 6, 2019 09:50

Address comment

96cf4d8

Merge remote-tracking branch 'upstream/master' into refactor-getUsedS…

e8dc564

…egmentsForInterval

jihoonson approved these changes Nov 6, 2019

View reviewed changes

jihoonson merged commit 5c0fc0a into apache:master Nov 6, 2019

leventov added this to the 0.17.0 milestone Nov 7, 2019

leventov deleted the refactor-getUsedSegmentsForInterval branch November 7, 2019 08:41

jon-wei mentioned this pull request Dec 28, 2019

0.17.0 release notes #9066

Closed

Conversation

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

Uh oh!

Description

Semantic changes, API changes, corrections

Fixes in tests

Refactorings and new utility methods

Refactoring of tests

Optimization

Uh oh!

nishantmonu51 left a comment

Choose a reason for hiding this comment

Uh oh!

egor-ryashin 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!

gianm commented Sep 30, 2019

Uh oh!

leventov commented Oct 11, 2019

Uh oh!

leventov commented Oct 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leventov Oct 30, 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!

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!

jihoonson left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

leventov commented Sep 20, 2019 •

edited

Loading

leventov Oct 30, 2019 •

edited

Loading