Refactor SQLMetadataSegmentManager; Change contract of REST methods in DataSourcesResource #7653

leventov · 2019-05-13T12:17:31Z

This PR is a large portion of #7306 PR extracted, but avoiding renaming classes so that Git doesn't lose track of files' history.

Description

Decouple `MetadataSegmentManager.start`/`stop` and `startPollingDatabasePeriodically`/`stopPollingDatabasePeriodically`.

Lifecycle's start() now doesn't start polling the database periodically.
stopPollingDatabasePeriodically() doesn't shutdown the executor, only the polling task. Next startPollingDatabasePeriodically() reuses the executor.

`SQLMetadataSegmentManager`: periodic and on-demand database polls

Made the MetadataSegmentManager's interface simpler and more robust by not exposing "periodic polls" implementation detail. Instead, when the Coordinator is not leading (hence MetadataSegmentManager is not polling the database periodically) and somebody (e. g. DataSourcesResource) queries the data from MetadataSegmentManager, an on-demand database poll is initiated under the hoods (if the previous on-demand poll was long time ago).

This change reworks the change #7447 by @gianm.

`DataSourcesResource` return code changes: returning numChangedSegments: N and segmentStateChanged: boolean

POST /druid/coordinator/v1/datasources/{dataSourceName}
DELETE /druid/coordinator/v1/datasources/{dataSourceName}
POST /druid/coordinator/v1/datasources/{dataSourceName}/markUnused
POST /druid/coordinator/v1/datasources/{dataSourceName}/markUsed

Now return a JSON object of the form {"numChangedSegments": N} instead of 204 (empty response) when no segments were changed. On the other hand, 500 (server error) is not returned instead of 204 (empty response).

There is a separate question of whether all endpoints in DataSourcesResource should return 404 on non-existent data source: see #7652. This aspect is not changed in this PR.

POST /druid/coordinator/v1/datasources/{dataSourceName}/segments/{segmentId}
DELETE /druid/coordinator/v1/datasources/{dataSourceName}/segments/{segmentId}

Now return a JSON object of the form {"segmentStateChanged": true/false}.

The change originates from this discussion: #7306 (comment)

REST API change: GET `/druid/coordinator/v1/metadata/datasources?includeDisabled` -> `includeUnused`

includeDisabled is still accepted, but a warning is emitted in a log. See changes in DataSourcesResource class.

FYI added a method JettyUtils.getQueryParam() to facilitate such parameter renames. (That's why this PR has Compatibility tag.)

`MetadataSegmentManager`: API refactor

enableDataSource -> markAsUsedAllNonOvershadowedSegmentsInDataSource
enableSegments(dataSource, interval) -> markAsUsedNonOvershadowedSegmentsInInterval
enableSegments(dataSource, Collection segmentIds) -> markAsUsedNonOvershadowedSegments(dataSource, Set segmentIds) throws UnknownSegmentIdException
enableSegment -> markSegmentAsUsed
removeDataSource -> markAsUnusedAllSegmentsInDataSource
disableSegments(dataSource, interval) -> markAsUnusedSegmentsInInterval
disableSegments(dataSource, Collection segmentIds) -> markSegmentsAsUnused(dataSource, Set segmentIds) throws UnknownSegmentIdException
removeSegment -> markSegmentAsUnused
getDataSource -> prepareImmutableDataSourceWithUsedSegments
getDataSources -> prepareImmutableDataSourcesWithAllUsedSegments
iterateAllSegments -> iterateAllUsedSegments
getAllDataSourceNames -> retrieveAllDataSourceNames

`SQLMetadataSegmentManager`: remove data from `dataSources` in `markAsUnusedSegmentsInInterval` and `markSegmentsAsUnused`

Fixed the logic added in #7490 to match the behavior of markAsUnusedAllSegmentsInDataSource. FYI @dampcake.

Optimizations

Fixed needlessly expensive ImmutableDruidDataSource.hashCode(), see Introduce published segment cache in broker #6901 (comment).
Removed needless toImmutableDataSource() (hence garbage) in DataSourcesResource.getDataSource().

This PR is Incompatible because it changes return codes of REST methods.

This PR is Development Blocker because it blocks #7306.

For reviewers: key changed classes are SQLMetadataSegmentManager and DataSourcesResource.

…n DataSourcesResource

…-refactor

leventov · 2019-05-21T15:37:34Z

@gianm @surekhasaharan @dampcake @egor-ryashin could you please review this PR (at least respective parts for which you are mentioned in the first message)?

gianm · 2019-05-22T05:15:15Z

I'll take a look at the SQLMetadataSegmentManager changes.

gianm

This review is only for SQLMetadataSegmentManager.java.

gianm · 2019-05-22T05:48:12Z

server/src/main/java/org/apache/druid/metadata/SQLMetadataSegmentManager.java

+  private @Nullable Future<?> periodicPollTaskFuture = null;
+
+  /** The number of times {@link #startPollingDatabasePeriodically} was called. */
+  private long startPollingCount = 0;


Should this be marked @GuardedBy("startStopPollLock")?

Thanks, marked

gianm · 2019-05-22T05:49:49Z

server/src/main/java/org/apache/druid/metadata/SQLMetadataSegmentManager.java

+   * {@link SQLMetadataRuleManager} also has a similar issue.
   */
-  private long currentStartOrder = -1;
+  private long currentStartPollingOrder = -1;


Should this be marked @GuardedBy("startStopPollLock")?

gianm · 2019-05-22T05:52:14Z

server/src/main/java/org/apache/druid/metadata/SQLMetadataSegmentManager.java


-  @Override
+  /**
+   * Don't confuse this method with {@link #startPollingDatabasePeriodically}. This is a lifecycle starting method to


Do you think it's worth including a sanity check that this method is only called one time?

I'm not sure it's called just once by our lifecycling logic. Most of @LifecycleStart methods in the project are idempotent. I think that's better than if they were throwing exceptions on non-first calls.

However, this method was lacking an idempotency check, I've added it.

gianm · 2019-05-22T06:48:06Z

server/src/main/java/org/apache/druid/metadata/SQLMetadataSegmentManager.java

-      catch (Exception e) {
-        log.makeAlert(e, "uncaught exception in segment manager polling thread").emit();
+      catch (Throwable t) {
+        log.makeAlert(t, "Uncaught exception in " + getClass().getName() + "'s polling thread").emit();


makeAlert supports embedded format strings, if you like to use that.

gianm · 2019-05-22T08:19:17Z

server/src/main/java/org/apache/druid/metadata/SQLMetadataSegmentManager.java

+   * the Coordinator is not the leader and therefore SqlSegmentsMetadata isn't polling the database periodically.
+   *
+   * The notion and the complexity of "on demand" database polls was introduced to simplify the interface of {@link
+   * MetadataSegmentManager} and guarantee that it always returns consistent and relatively up-to-date data from methods like


With this interface change, can callers still tell if there has been a poll since the start of the leadership epoch?

(I think it's nice if they can, for the reason mentioned in #7595 (comment) -- preventing leadership epoch N from using an older snapshot than leadership epoch N-1, due to its polling being out of date.)

As you noted below, this is guaranteed, indeed. I've extended the comment.

However, note that there is actually no semantic difference between periodic and on-demand database polls because both maintain the same invariant that the results no older than periodicPollDelay are used. The main difference is in performance: since on-demand polls are irregular and happen in the context of the thread wanting to access the dataSources, that may cause delays in the logic. On the other hand, periodic polls are decoupled into exec and dataSources-accessing methods should be generally "wait free" for database polls.

I've added this into the comment too.

gianm · 2019-05-22T08:26:23Z

server/src/main/java/org/apache/druid/metadata/SQLMetadataSegmentManager.java

-      final Collection<String> segmentIds,
-      final Handle handle
-  )
+  private boolean awaitPeriodicOrFreshOnDemandDatabasePoll()


How about calling this awaitLatestDatabasePoll()? It seems more intuitive.

gianm · 2019-05-22T08:26:48Z

server/src/main/java/org/apache/druid/metadata/SQLMetadataSegmentManager.java

-        .orElseThrow(() -> new UnknownSegmentIdException(StringUtils.format("Cannot find segment id [%s]", segmentId)))
-    )
-    .collect(Collectors.toList());
+    DatabasePoll latestDatabasePoll = this.latestDatabasePoll;


It looks like the idea is that if we are in the midst of a leadership epoch, then:

isPollingDatabasePeriodically would return true

latestDatabasePoll will be a PeriodicDatabasePoll

we are guaranteed to await for that PeriodicDatabasePoll to complete its first poll, if awaitPeriodicOrFreshOnDemandDatabasePoll is called after the leadership epoch starts

Is that right? If so it should provide the property I mentioned in another comment (preventing leadership epoch N from using an older snapshot than leadership epoch N-1). If so IMO it'd be nice to have some comments or assertions about these class invariants.

Added, see #7653 (comment)

gianm · 2019-05-22T10:17:11Z

server/src/main/java/org/apache/druid/metadata/SQLMetadataSegmentManager.java

+    }
+  }
+
+  private List<DataSegment> retreiveUnusedSegments(


retrieve (spelling)

Thanks, fixed

gianm · 2019-05-22T10:17:47Z

server/src/main/java/org/apache/druid/metadata/SQLMetadataSegmentManager.java

+    return segments;
+  }
+
+  private Iterator<DataSegment> retreiveUsedSegmentsOverlappingIntervals(


retrieve (spelling)

…-refactor

surekhasaharan · 2019-05-27T20:08:38Z

docs/content/operations/api-reference.md


-Disables a segment.
+Marks as unused a segment of a data source. Returns a JSON object of the form `{"segmentStateChanged": <boolean>"}` with
+the boolean indicating if the state of the segment has been changed (that is, the segment was marked as used) as the


"segment was marked as used" -> "segment was marked as unused"

Thanks, fixed

surekhasaharan · 2019-05-27T23:06:29Z

server/src/main/java/org/apache/druid/metadata/SQLMetadataSegmentManager.java

+        dataSource.addSegment(segment);
+      }
+      String s = segment.getId().toString();
+      segmentIdsToMarkAsUsed.add(s);


nit : rename s to segmentId or get rid of s and do it inline segmentIdsToMarkAsUsed.add(segment.getId().toString())

surekhasaharan · 2019-05-28T05:07:07Z

server/src/main/java/org/apache/druid/metadata/SQLMetadataSegmentManager.java

                        }
                        catch (IOException e) {
                          log.makeAlert(e, "Failed to read segment from db.").emit();
+                          // If one entry is database is corrupted, doPoll() should continue to work overall. See


nit: If one entry "is" -> "in" database ...

Thanks, fixed

jon-wei · 2019-05-29T23:39:55Z

Reviewed the REST API changes in DataSourcesResource and MetadataResource, those LGTM.

…-refactor

egor-ryashin · 2019-05-23T21:22:00Z

server/src/main/java/org/apache/druid/client/indexing/ClientCompactQueryTuningConfig.java

  private final Long pushTimeout;

  public static ClientCompactQueryTuningConfig from(
-      @Nullable UserCompactTuningConfig userCompactTuningConfig,


IMHO, this is too local a variable for this renaming effort (just a note).

egor-ryashin · 2019-06-03T21:31:57Z

server/src/main/java/org/apache/druid/client/ImmutableDruidDataSource.java

  public int hashCode()
  {
-    return Objects.hash(name, properties, idToSegments);
+    return Objects.hash(name, properties);


Could you comment on this? There are too many places ImmutableDruidDataSource is used, so I couldn't check in a reasonable time the purpose and correction of that change.

See #6901 (comment). This is actually noted in the PR description, "Optimizations" section.

egor-ryashin · 2019-06-04T19:57:08Z

server/src/main/java/org/apache/druid/client/ImmutableDruidServer.java

   * once rather than several times.
   */
-  public Collection<DataSegment> getLazyAllSegments()
+  public Collection<DataSegment> iterateAllSegments()


Somewhat misleading as it doesn't iterate but creates a collection.

This is intentional: collections returned from all methods whose names start with iterate- are not expected to be stored anywhere. They are only expected to be consumed immediately in a for-each statement, forEach() call or a Stream pipeline.

egor-ryashin · 2019-06-05T14:21:56Z

server/src/main/java/org/apache/druid/metadata/SQLMetadataSegmentManager.java

+   * Marker interface for objects stored in {@link #latestDatabasePoll}. See the comment for that field for details.
   */
-  private final ReentrantReadWriteLock startStopLock = new ReentrantReadWriteLock();
+  private interface DatabasePoll


I wonder why the marker interface was introduced?

Mainly for the ability to make Javadoc references.

…-refactor

leventov · 2019-07-16T13:10:12Z

@egor-ryashin could you please check this PR again?

jihoonson · 2019-07-18T00:48:18Z

core/src/main/java/org/apache/druid/timeline/partition/PartitionHolder.java

+  public boolean add(PartitionChunk<T> chunk)
  {
-    holderSet.add(chunk);
+    return holderMap.putIfAbsent(chunk, chunk) == null;


I believe this is a bug. PartitionChunk is one of most wacky things in Druid when it comes to equals() and hashCode() implementations. It considers only the partitionId and ignores the object in the PartitionChunk. The intention here is to always replace the existing one with the new one. I think this behavior is to swap realtime segments with published and available ones in historicals. I know this is confusing and must be addressed in the future.

Ah sorry. I was confused. Any new value shouldn't overwrite the existing value, so this looks correct.

jihoonson · 2019-07-18T00:50:22Z

core/src/main/java/org/apache/druid/timeline/VersionedIntervalTimeline.java

      Comparators.intervalsByStartThenEnd()
  );
  private final Map<Interval, TreeMap<VersionType, TimelineEntry>> allTimelineEntries = new HashMap<>();
+  private final AtomicInteger numObjects = new AtomicInteger();


VersionedIntervalTimeline is not thread-safe. Should this be just an integer? I'm not sure how you're planning to use VersionedIntervalTimeline.

Or, this variable is used only in iterateAllObjects. Should it return a collection? Or can we change the return type to Stream or Iterator? With this change, we don't have to track the number of objects in the timeline.

VersionedIntervalTimeline actually has to be thread-safe: see #7285.

It's more convenient for users of the class if the method returns a Collection rather than Iterable or Stream: it makes possible both for-each iteration and a stream pipeline. Although currently this method is used in just one place with forEach() call, I don't see much value of over-specializing for this case now.

As aforementioned, we don't have to have numObjects if iterateAllObjects() returns an Iterable or Stream. As you noted, the only user is calling forEach() on the result, it doesn't have to be a collection. As far as I remember, we agreed on simplifying unnecessarily generalized code.

I actually discovered a use of numObjects in #7306, from where this PR is partially extracted: https://github.com/apache/incubator-druid/pull/7306/files#diff-b3571a010cbbb3cb648676a89936a51fR277

clintropolis · 2019-07-18T21:20:35Z

server/src/main/java/org/apache/druid/server/http/MetadataResource.java

-    final Set<SegmentId> overshadowedSegments = Optional
-        .ofNullable(metadataSegmentManager.getOvershadowedSegments())
-        .orElse(Collections.emptySet());
+    DataSourcesSnapshot dataSourcesSnapshot = segmentsMetadata.getSnapshotOfDataSourcesWithAllUsedSegments();


With an empty database that has no segments I'm getting a null pointer exception because the dataSourceSnapshot set here is null.

Allowing an empty database to still set a snapshot, by not returning here and instead making an 'empty' snapshot remedies this issue, but I'm not sure if it is the right fix because it's different than previous behavior, where a null would be retained until segments existed.

raised #8106 if the approach to fixing is legitimate

Thanks for finding this and proposing a fix

jihoonson · 2019-08-06T18:44:00Z

server/src/main/java/org/apache/druid/metadata/SQLMetadataSegmentManager.java

+   * periodically (see {@link PeriodicDatabasePoll}, {@link #startPollingDatabasePeriodically}, {@link
+   * #stopPollingDatabasePeriodically}) or "on demand" (see {@link OnDemandDatabasePoll}), when one of the methods that
+   * accesses {@link #dataSourcesSnapshot}'s state (such as {@link #getImmutableDataSourceWithUsedSegments}) is
+   * called when the Coordinator is not the leader and therefore SqlSegmentsMetadata isn't polling the database


I don't understand this explanation. All API calls to non-leaders should be redirected to the leader. How could database polling happen for non-leaders?

"All API calls to non-leaders should be redirected to the leader" - you mean internally, within a Druid cluster, by a non-leader Coordinator, or externally, API callers should subscribe to the leader updates in ZK and always call to the leading Coordinator?

All methods in SQLMetadataSegmentManager are called by DruidCoordinator or MetadataResource. DruidCoordinator has its lifecycle and doesn't do any coordinator work if it's leading.

All HTTP calls to MetadataResource of non-leader from outside Druid should be automatically redirected to the leader.

Interesting, thanks. I didn't know about this redirect.

It still seems to me that SQLMetadataSegmentManager can be called from MetadataResource outside of leadership between the moment when this Coordinator becomes the leader (DruidLeaderSelector.getCurrentLeader() returns this Coordinator's id) and SQLMetadataSegmentManager gets notified about that in DruidCoordinator.becomeLeader(). (See also the Javadoc for LeaderLatchListener which describes another intersting race condition.)

If this is true, I think it's worth reflecting in some comments in code. I will think about where to put this.

Good point and the javadoc of LeaderLatchListener does sound interesting.

It still seems to me that SQLMetadataSegmentManager can be called from MetadataResource outside of leadership between the moment when this Coordinator becomes the leader (DruidLeaderSelector.getCurrentLeader() returns this Coordinator's id) and SQLMetadataSegmentManager gets notified about that in DruidCoordinator.becomeLeader().

My feeling about this is DruidCoordinator.isLeader() and DruidCoordinator.getCurrentLeader() could be wrong. Even when DruidLeaderSelector says this coordinator is leading now, DruidCoordinator.isLeader() shouldn't return true until DruidCoordinator.becomeLeader() is finished. A similar issue was in the overlord and now it fixed as here (TaskMaster.getCurrentLeader() looks still wrong though).

Refactor SQLMetadataSegmentManager; Change contract of REST methods i…

fb11564

…n DataSourcesResource

leventov added Incompatible Design Review Compatibility Development Blocker Refactoring labels May 13, 2019

leventov requested review from egor-ryashin, gianm and surekhasaharan May 13, 2019 12:17

leventov added 5 commits May 14, 2019 15:33

Style fixes

a0f72bd

Unused imports

2733f8e

Fix tests

32382b1

Merge remote-tracking branch 'upstream/master' into sqlSegmentManager…

9a52dd0

…-refactor

Fix style

366551b

leventov mentioned this pull request May 21, 2019

Optimize overshadowed segments computation #7595

Merged

gianm reviewed May 22, 2019

View reviewed changes

leventov added 3 commits May 24, 2019 15:20

Comments

f035751

Comment fix

89ba356

Merge remote-tracking branch 'upstream/master' into sqlSegmentManager…

5e18bd6

…-refactor

surekhasaharan reviewed May 28, 2019

View reviewed changes

Remove unresolvable Javadoc references; address comments

f59f5bc

Merge remote-tracking branch 'upstream/master' into sqlSegmentManager…

af941e5

…-refactor

egor-ryashin requested changes Jun 7, 2019

View reviewed changes

leventov added 4 commits June 10, 2019 15:14

Add comments to ImmutableDruidDataSource

8bac8ef

Merge with master

8915bc0

Merge remote-tracking branch 'upstream/master' into sqlSegmentManager…

727d730

…-refactor

Fix bad web-console merge

bd97056

leventov added 5 commits July 12, 2019 18:08

Fixes in api-reference.md

0e39fb4

Rename in DruidCoordinatorRuntimeParams

82d287d

Fix compilation

ec805b4

Residual changes

cb456cf

Merge remote-tracking branch 'upstream/master' into sqlSegmentManager…

68fa4e4

…-refactor

egor-ryashin approved these changes Jul 17, 2019

View reviewed changes

leventov merged commit ceb9699 into apache:master Jul 17, 2019

leventov deleted the sqlSegmentManager-refactor branch July 17, 2019 14:18

jihoonson reviewed Jul 18, 2019

View reviewed changes

clintropolis reviewed Jul 18, 2019

View reviewed changes

clintropolis mentioned this pull request Jul 18, 2019

fix npe with sql metadata manager polling and empty database #8106

Merged

2 tasks

leventov mentioned this pull request Aug 1, 2019

Reconcile terminology and method naming to 'used/unused segments'; Rename MetadataSegmentManager to MetadataSegmentsManager #7306

Merged

jihoonson reviewed Aug 6, 2019

View reviewed changes

leventov added this to the 0.16.0 milestone Aug 6, 2019

leventov added the Release Notes label Aug 6, 2019

leventov mentioned this pull request Aug 8, 2019

DruidCoordinator.isLeader(), getCurrentLeader() and TaskMaster.getCurrentLeader() are wrong #8263

Open

clintropolis mentioned this pull request Aug 21, 2019

0.16.0-incubating release notes #8369

Closed

Refactor SQLMetadataSegmentManager; Change contract of REST methods in DataSourcesResource #7653

Refactor SQLMetadataSegmentManager; Change contract of REST methods in DataSourcesResource #7653

Uh oh!

Conversation

leventov commented May 13, 2019

Description

Decouple MetadataSegmentManager.start/stop and startPollingDatabasePeriodically/stopPollingDatabasePeriodically.

SQLMetadataSegmentManager: periodic and on-demand database polls

DataSourcesResource return code changes: returning numChangedSegments: N and segmentStateChanged: boolean

REST API change: GET /druid/coordinator/v1/metadata/datasources?includeDisabled -> includeUnused

MetadataSegmentManager: API refactor

SQLMetadataSegmentManager: remove data from dataSources in markAsUnusedSegmentsInInterval and markSegmentsAsUnused

Optimizations

Uh oh!

leventov commented May 21, 2019

Uh oh!

gianm commented May 22, 2019

Uh oh!

gianm 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

Uh oh!

leventov May 24, 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!

jon-wei commented May 29, 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

Decouple `MetadataSegmentManager.start`/`stop` and `startPollingDatabasePeriodically`/`stopPollingDatabasePeriodically`.

`SQLMetadataSegmentManager`: periodic and on-demand database polls

`DataSourcesResource` return code changes: returning numChangedSegments: N and segmentStateChanged: boolean

REST API change: GET `/druid/coordinator/v1/metadata/datasources?includeDisabled` -> `includeUnused`

`MetadataSegmentManager`: API refactor

`SQLMetadataSegmentManager`: remove data from `dataSources` in `markAsUnusedSegmentsInInterval` and `markSegmentsAsUnused`

leventov May 24, 2019 •

edited

Loading

jihoonson Jul 18, 2019 •

edited

Loading

jihoonson Jul 18, 2019 •

edited

Loading

leventov Jul 18, 2019 •

edited

Loading