Handoff should ignore segments that are dropped by drop rules #6676

QiuMM · 2018-11-28T09:20:10Z

QiuMM · 2018-11-28T14:26:50Z

...va/org/apache/druid/segment/realtime/plumber/CoordinatorBasedSegmentHandoffNotifierTest.java

@@ -21,7 +21,6 @@

 import com.google.common.collect.Sets;
 import com.google.common.util.concurrent.MoreExecutors;
-import junit.framework.Assert;


It is deprecated, so I used org.junit.Assert to replace it.

jihoonson

Hi @QiuMM, thanks for raising this PR! I left a couple of comments.

jihoonson · 2018-11-28T18:57:33Z

...n/java/org/apache/druid/segment/realtime/plumber/CoordinatorBasedSegmentHandoffNotifier.java

@@ -95,6 +95,17 @@ void checkForSegmentHandoffs()
        Map.Entry<SegmentDescriptor, Pair<Executor, Runnable>> entry = itr.next();
        SegmentDescriptor descriptor = entry.getKey();
        try {
+          if (coordinatorClient.isSpecificIntervalDroppedByRule(dataSource, descriptor.getInterval())) {


This will require more HTTP calls. I think it's better to use a single API to check this and get the server view below.
That API should be able to notify caller that the handoff for the requested segment will never happen.

We might improve the existing coordinator API, DataSourceResource.getSegmentDataSourceSpecificInterval() or we can add a new one for it.

Sounds good, I'll try it.

jihoonson · 2018-11-28T19:02:05Z

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

+    for (Rule rule : rules) {
+      if (rule.appliesTo(theInterval, now)) {
+        if (rule instanceof DropRule) {
+          dropped = true;


This looks not correct. First of all, rules can be empty. This effectively makes any segments to not be loaded into historicals. Also, only the first matched rule is applied. Even if there is a matched drop rule, a segment can be loaded if any load rule matches before checking the drop rule.

Below is the whole loop:

boolean dropped = false; for (Rule rule : rules) { if (rule.appliesTo(theInterval, now)) { if (rule instanceof DropRule) { dropped = true; } break; } }

When found the first matched rule, the loop will break. So there is no doubt that only the first matched rule is applied.

I'll update this PR to handle the case that the rules is empty.

Oh you're right. Thanks. I missed the break.

QiuMM · 2018-12-04T11:00:41Z

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

@@ -620,23 +627,49 @@ private ImmutableDruidDataSource getDataSource(final String dataSourceName)
  @Path("/{dataSourceName}/intervals/{interval}/serverview")
  @Produces(MediaType.APPLICATION_JSON)
  @ResourceFilters(DatasourceResourceFilter.class)
-  public Response getSegmentDataSourceSpecificInterval(
+  public Response getSegmentServerview(


Changed the method name to getSegmentServerview since I think it makes more sense, and there is another method using the same name getSegmentDataSourceSpecificInterval in this class.

jihoonson · 2018-12-10T23:24:49Z

@QiuMM, thank you for updating quickly and sorry for the delayed review.

I've checked the latest changes, and would like to make one more suggestion. The changed coordinator API (getSegmentServerview) now returns a list of ImmutableSegmentLoadInfos with a dropped flag. This is an incompatible change because this is a documented public API and its return type has been changed. (Sorry, I didn't notice that before and recommended to change the existing one.) It means this PR should be labelled Design Review.

However, I think it's not necessary and there might be an easier way to fix this issue. I would suggest to add a new API for only this internal use case without changing the existing one. If you do this, you don't have to worry about compatibility. Also, if the new API just returns a boolean result which indicates that the given segment is handed off, is waiting for hand off, or will never be handed off, the API is lighter than returning the entire server view which is better in terms of efficiency. What do you think?

QiuMM · 2018-12-11T03:24:33Z

@jihoonson thanks for your review, your suggestion sounds good to me, I'll update it ASAP.

gianm · 2018-12-20T23:20:00Z

Hey @QiuMM - do you think you'll have a chance to come back to this one? Thanks!!

QiuMM · 2018-12-21T04:21:08Z

@gianm @jihoonson sorry for the late because I'm busy with my work, I'll update this pr on this weekend.

jihoonson

@QiuMM thanks for the update. I left two more comments.

jihoonson · 2019-01-02T21:20:36Z

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

+  @Path("/{dataSourceName}/handoffComplete")
+  @Produces(MediaType.APPLICATION_JSON)
+  @ResourceFilters(DatasourceResourceFilter.class)
+  public Response isHandOffComplete(


So, it returns true when the segment will be never loaded or is already loaded. Otherwise, it returns `false. Would you add a javadoc about this?

jihoonson · 2019-01-02T21:23:07Z

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

+      return Response.ok(false).build();
+    }
+    catch (Exception e) {
+      return Response.serverError().entity(ImmutableMap.of("error", e.toString())).build();


Probably logging the full stack trace of exception would help as well?

jihoonson · 2019-01-03T17:14:07Z

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

@@ -713,6 +717,7 @@ public Response isHandOffComplete(
      return Response.ok(false).build();
    }
    catch (Exception e) {
+      log.error(e, "");


Please add a proper error message like "Error while handling handoffComplete request"

jihoonson

Thanks @QiuMM. LGTM.

glasser · 2019-01-08T00:02:54Z

Thanks @QiuMM! This issue caused us major confusion in our initial rollout of Druid (for better or for worse, our APM service uses untrusted timestamps from external users as Druid timestamps). This is huge for anybody receiving untrusted timestamps.

gianm · 2019-01-30T18:49:45Z

Unless I am missing something, won't this patch break rolling updates? New tasks won't be able to talk to old coordinators, and typically, we advise updating coordinators last: http://druid.io/docs/latest/operations/rolling-updates.html. If so, then there should be some kind of fallback to using the old API if the new one isn't available.

jihoonson · 2019-01-30T20:21:00Z

@gianm thanks for catching it. I'll make a fix.

qiumingming.2018 added 3 commits November 28, 2018 17:18

Handoff should ignore segments that are dropped by drop rules

1781afd

fix travis-ci

69c432e

fix tests

956ab63

QiuMM commented Nov 28, 2018

View reviewed changes

jihoonson reviewed Nov 28, 2018

View reviewed changes

fjy added this to the 0.13.1 milestone Dec 3, 2018

address comments

0171c69

QiuMM commented Dec 4, 2018

View reviewed changes

remove line added by accident

ec04ec9

gianm assigned jihoonson Dec 20, 2018

address comments

7cfbe8d

jihoonson reviewed Jan 2, 2019

View reviewed changes

add javadoc and logging the full stack trace of exception

65d84ed

jihoonson reviewed Jan 3, 2019

View reviewed changes

add error message

3902446

jihoonson approved these changes Jan 4, 2019

View reviewed changes

jihoonson merged commit 8ebb7b5 into apache:master Jan 7, 2019

jihoonson mentioned this pull request Jan 31, 2019

Fall back to the old coordinator API for checking segment handoff if new one is not supported #6966

Merged

jon-wei mentioned this pull request Feb 22, 2019

0.14.0-incubating release notes #7126

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handoff should ignore segments that are dropped by drop rules #6676

Handoff should ignore segments that are dropped by drop rules #6676

QiuMM commented Nov 28, 2018

QiuMM Nov 28, 2018 •

edited

Loading

jihoonson left a comment

jihoonson Nov 28, 2018

QiuMM Nov 29, 2018

jihoonson Nov 28, 2018

QiuMM Nov 29, 2018 •

edited

Loading

jihoonson Nov 29, 2018

QiuMM Dec 4, 2018 •

edited

Loading

jihoonson commented Dec 10, 2018

QiuMM commented Dec 11, 2018

gianm commented Dec 20, 2018

QiuMM commented Dec 21, 2018

jihoonson left a comment

jihoonson Jan 2, 2019

QiuMM Jan 3, 2019

jihoonson Jan 2, 2019

QiuMM Jan 3, 2019

jihoonson Jan 3, 2019

QiuMM Jan 4, 2019

jihoonson left a comment

glasser commented Jan 8, 2019

gianm commented Jan 30, 2019

jihoonson commented Jan 30, 2019

Handoff should ignore segments that are dropped by drop rules #6676

Handoff should ignore segments that are dropped by drop rules #6676

Conversation

QiuMM commented Nov 28, 2018

QiuMM Nov 28, 2018 • edited Loading

Choose a reason for hiding this comment

jihoonson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QiuMM Nov 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QiuMM Dec 4, 2018 • edited Loading

Choose a reason for hiding this comment

jihoonson commented Dec 10, 2018

QiuMM commented Dec 11, 2018

gianm commented Dec 20, 2018

QiuMM commented Dec 21, 2018

jihoonson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jihoonson left a comment

Choose a reason for hiding this comment

glasser commented Jan 8, 2019

gianm commented Jan 30, 2019

jihoonson commented Jan 30, 2019

QiuMM Nov 28, 2018 •

edited

Loading

QiuMM Nov 29, 2018 •

edited

Loading

QiuMM Dec 4, 2018 •

edited

Loading