Automatic pendingSegments cleanup by jihoonson · Pull Request #5149 · apache/druid

jihoonson · 2017-12-11T13:02:18Z

With this patch, the coordinator periodically deletes pendingSegments from the pendingSegments table. You can test by adding druid.coordinator.kill.pendingSegments.on=true to your coordinator configuration file.

Additionally, I added TaskStatusPlus which contains taskId, createdTime, queueInsertionTime, taskState, runningDuration, and taskLocation. We currently have 4 similar classes, i.e., TaskStatus, TaskRunnerWorkItem, TaskResponseObject in OverlordResource, and TaskResponseObject for integration tests, but they are designed to be used in some specific classes. I would love to add a new one to represent all these classes and remove others, but it will cause a lot of code changes which makes difficult to be done together in this PR. So, in this PR,

TaskResponseObjects are replaced by TaskStatusPlus and moved to druid-api. This is to use the same class when coordinators call overlord APIs.
The Status enum in TaskStatus is extracted as a separate TaskState enumeration and moved to druid-api.
TaskLocation is moved to druid-api.

This change is

jon-wei

Did an initial review, will revisit this later today

jon-wei · 2017-12-13T19:39:19Z

+    }
+  }
+
+  private List<TaskStatus> getRecentlyFinishedTaskSTatusesSince(long start, Ordering<TaskStuff> createdDateDesc)


There's an extra capitalized T:

getRecentlyFinishedTaskSTatusesSince > getRecentlyFinishedTaskStatusesSince

Thanks. Fixed.

jon-wei · 2017-12-13T20:15:01Z

+      for (ImmutableDruidDataSource dataSource : params.getDataSources()) {
+        if (!params.getCoordinatorDynamicConfig().getKillPendingSegmentsSkipList().contains(dataSource.getName())) {
+          log.info(
+              "Kill pendingSegments created until [%s] for dataSource[%s]",


The string format arguments are in reverse order here

Thanks. Fixed.

jon-wei · 2017-12-13T20:16:28Z

                DruidCoordinatorSegmentKiller.class
+            ).addConditionBinding(
+                "druid.coordinator.kill.pendingSegments.on",
+                predicate -> Objects.equals(predicate, "true"),


Maybe use Predicates.equalTo("true") to be consistent with the other bindings here

jon-wei · 2017-12-13T20:36:29Z

  }

+  @JsonProperty
+  public Set<String> getKillPendingSegmentsSkipList()


Can you add a comment that explains this is used to prevent automatic pending segment deletion for some datasources?

I was a bit confused initially and thought the skip list was referring to this: https://en.wikipedia.org/wiki/Skip_list

jon-wei · 2017-12-13T21:03:38Z

+
+    Preconditions.checkArgument(
+        !deleteInterval.overlaps(activeTaskInterval),
+        "Cannot delete pendingSegments because there is at least one running task created at %s",


hm, I wonder if throwing an exception here is too aggressive, if I understand correctly:

Since tasks can take a varying amount of time to complete, it seems like it wouldn't be too uncommon to have a situation where there are active tasks, created before the most recently completed task, that are still running

Would it be better to just return 0 here without an exception, or maybe use minCreatedDateOfActiveTasks as the end of the pending segment deletion interval?

Good point. I changed to gather all incomplete tasks and the last complete task and find the earliest createdTime. This makes less exceptions occur when the coordinator kills pending segments.

I think throwing an exception here is fine now because it rarely occurs 1) if there is a bug in DruidCoordinatorCleanupPendingSegments and 2) humans call the overlord API directly with a wrong interval.

…ng-segments

gianm

I also believe that #5161 is exacerbated by this patch (as it makes it possible to reissue a previously-pending segment identifier, which could cause a similar issue to the race from #5161). But I think that's fine, since the fix to #5161 should involve overwriting, which would fix both problems.

gianm · 2017-12-13T16:32:11Z

      Interval tryInterval,
      Interval rowInterval,
-      boolean logOnFail,
-      boolean skipSegmentLineageCheck


skipSegmentLineageCheck can be removed from IndexerMetadataStorageConnector's allocatePendingSegment too.

Would you elaborate on this? I guess you mean IndexerMetadataStorageCoordinator, but skipSegmentLineageCheck is used in IndexerSQLMetadataStorageCoordinator.allocatePendingSegment().

gianm · 2017-12-13T16:34:01Z

+    }
+  }
+
+  private List<TaskStatus> getRecentlyFinishedTaskSTatusesSince(long start, Ordering<TaskStuff> createdDateDesc)


Capitalization is a bit weird on TaskSTatuses

gianm · 2017-12-13T16:59:58Z

-          }
-        }
-    );
+    final List<TaskStatusPlus> completeTasks = recentlyFinishedTasks


Please retain the comment // Would be nice to include the real created date, but the TaskStorage API doesn't yet allow it. (or even make it possible!)

Added the comment. I'm thinking to refactor some classes related to TaskStatus. The APIs to get createdDate and queueInsertionTime will be added when it's done!

gianm · 2017-12-14T00:07:26Z

+  @Produces(MediaType.APPLICATION_JSON)
+  public Response killPendingSegments(
+      @PathParam("dataSource") String dataSource,
+      @QueryParam("interval") String deleteIntervalString,


Would it work to make this an Interval rather than String? We do have a jackson deserializer set up from json strings to Druid Intervals.

The javadoc of QueryParam is saying

The type T of the annotated parameter, field or property must either:

Be a primitive type

Have a constructor that accepts a single String argument

Have a static method named valueOf or fromString that accepts a single String argument (see, for example, Integer.valueOf(String))

Be List, Set or SortedSet, where T satisfies 2 or 3 above. The resulting collection is read-only.

I guess the deleteInterval should be passed in the HTTP message body rather than QueryParam or PathParam (PathParm also has similar requirements with QueryParam) to make this type-safe. If so, the as-is looks better to me because it's simple.

gianm · 2017-12-14T00:08:25Z

+        .map(task -> taskStorageQueryAdapter.getCreatedTime(task.getId()))
+        .min(Comparator.naturalOrder());
+
+    final Interval activeTaskInterval = new Interval(


I think we should include some kind of buffer zone here to account for the fact that clocks are not guaranteed to be in sync. I'm not sure how much grace period is needed but probably somewhere between 10 minutes and 24 hours. What do you think?

I guess you mean we need a buffer when the coordinator kills pending segments. Or do you mean the buffer is needed when IndexerMetadataStorageAdapter checks the given interval overlaps the createdDate of any running tasks?
The former one sounds good. I added a buffer of 24 hours and now the endDate of the interval for killing pendingSegments is decided by min(createdDates of running/pending/waiting/complete tasks, DateTimes.nowUtc()).

gianm · 2017-12-14T20:40:26Z

+    );
+
+    if (!authResult.isAllowed()) {
+      throw new ForbiddenException(authResult.toString());


authResult.getMessage() would make more sense I think. Also, now that I look at AuthResult, it seems that setMessage is never called and so it could be removed and message made final. It's unrelated to this patch but still a nice cleanup.

jihoonson · 2017-12-19T05:57:53Z

I also believe that #5161 is exacerbated by this patch (as it makes it possible to reissue a previously-pending segment identifier, which could cause a similar issue to the race from #5161). But I think that's fine, since the fix to #5161 should involve overwriting, which would fix both problems.

@gianm Good point. I agree.

jon-wei · 2017-12-20T04:07:06Z

LGTM, can you fix the CI errors?

pdeva · 2018-03-31T01:27:23Z

note that this configuration is not documented in the 0.12.0 docs

jihoonson · 2018-04-02T18:10:12Z

Thank you for the report! Raised #5563.

glasser · 2019-11-12T18:10:00Z

+    }
+    createdTimes.sort(Comparators.naturalNullsFirst());
+
+    // There should be at least one createdTime because the current time is added to the 'createdTimes' list if there


Is this comment accurate? I don't see where it's implemented, which suggests that this helper can crash in a brand new cluster.

jihoonson added 2 commits December 11, 2017 21:40

PendingSegments cleanup

3769f7b

fix build

68a39be

jihoonson mentioned this pull request Dec 12, 2017

Automatic compaction by coordinators #5102

Merged

jon-wei reviewed Dec 13, 2017

View reviewed changes

jihoonson added 5 commits December 14, 2017 11:05

Merge branch 'master' of https://github.com/druid-io/druid into pendi…

00050e2

…ng-segments

address comments

dbc2b77

address comments

2ebdaed

Merge branch 'master' of https://github.com/druid-io/druid into pendi…

c9abcd1

…ng-segments

fix potential npe

017fce3

gianm reviewed Dec 19, 2017

View reviewed changes

address comments

3ac2603

jihoonson added 3 commits December 20, 2017 20:33

fix build

599b349

fix test

f8b4a7e

fix test

5ae915c

jon-wei merged commit 9199d61 into apache:master Dec 20, 2017

jon-wei added this to the 0.12.0 milestone Jan 5, 2018

jon-wei mentioned this pull request Jan 5, 2018

[WIP] Druid 0.12.0 release notes #5211

Closed

jihoonson mentioned this pull request Jan 10, 2018

Better index for segments and pendingSegments table #4936

Closed

This was referenced Jan 23, 2018

Clean up pendingSegments table #3565

Closed

add interface to auto clean mysql pendingSegments table #3831

Closed

jihoonson mentioned this pull request Apr 2, 2018

Missing doc for automatic pendingSegments cleanup #5563

Closed

jihoonson mentioned this pull request Jul 10, 2018

Kafka Indexing Service lagging every hour #5979

Closed

glasser reviewed Nov 12, 2019

View reviewed changes

Conversation

jihoonson commented Dec 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jon-wei 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!

Choose a reason for hiding this comment

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!

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 commented Dec 19, 2017

Uh oh!

jon-wei commented Dec 20, 2017

Uh oh!

pdeva commented Mar 31, 2018

Uh oh!

jihoonson commented Apr 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jihoonson commented Dec 11, 2017 •

edited

Loading

jihoonson commented Apr 2, 2018 •

edited

Loading