ParallelIndexSubTask: support ingestSegment in delegating factories by glasser · Pull Request #7089 · apache/druid

glasser · 2019-02-19T07:28:50Z

IndexTask had special-cased code to properly send a TaskToolbox to a
IngestSegmentFirehoseFactory that's nested inside a CombiningFirehoseFactory,
but ParallelIndexSubTask didn't.

This change refactors IngestSegmentFirehoseFactory so that it doesn't need a
TaskToolbox; it instead gets a CoordinatorClient and a SegmentLoaderFactory
directly injected into it.

This also refactors SegmentLoaderFactory so it doesn't depend on
an injectable SegmentLoaderConfig, since its only method always
replaces the preconfigured SegmentLoaderConfig anyway.
This makes it possible to use SegmentLoaderFactory without setting
druid.segmentCaches.locations to some dummy value.

Another goal of this PR is to make it possible for IngestSegmentFirehoseFactory
to list data segments outside of connect() --- specifically, to make it a
FiniteFirehoseFactory which can query the coordinator in order to calculate its
splits. See #7048.

This also adds missing datasource name URL-encoding to an API used by
CoordinatorBasedSegmentHandoffNotifier.

glasser · 2019-02-19T08:57:36Z

Note that this PR is integration-tested by ITIndexerTest.

Please note two FIXME comments in the PR.

My intention is to squash the two commits of the PR together (and keep the PR description as the commit message). I thought reviewers might like to see my initial attempt which injected a SegmentLoaderFactory instead of directly creating a SegmentLoaderLocalCacheManager. That commit led to this error in ITIndexerTest:

Can not construct instance of org.apache.druid.indexing.firehose.IngestSegmentFirehoseFactory, problem: Unable to provision, see the following errors:

1) druid.segmentCache.locations - may not be empty
  at org.apache.druid.guice.JsonConfigProvider.bind(JsonConfigProvider.java:151) (via modules: com.google.inject.util.Modules$OverrideModule -> com.google.inject.util.Modules$OverrideModule -> org.apache.druid.guice.StorageNodeModule)
  at org.apache.druid.guice.JsonConfigProvider.bind(JsonConfigProvider.java:151) (via modules: com.google.inject.util.Modules$OverrideModule -> com.google.inject.util.Modules$OverrideModule -> org.apache.druid.guice.StorageNodeModule)
  while locating com.google.common.base.Supplier<org.apache.druid.segment.loading.SegmentLoaderConfig>
  at org.apache.druid.guice.ConfigProvider.bind(JsonConfigProvider.java:152) (via modules: com.google.inject.util.Modules$OverrideModule -> com.google.inject.util.Modules$OverrideModule -> org.apache.druid.guice.StorageNodeModule)
  while locating org.apache.druid.segment.loading.SegmentLoaderConfig
    for the 2nd parameter of org.apache.druid.segment.loading.SegmentLoaderLocalCacheManager.<init>(SegmentLoaderLocalCacheManager.java:67)
  while locating org.apache.druid.segment.loading.SegmentLoaderLocalCacheManager
    for the 1st parameter of org.apache.druid.indexing.common.SegmentLoaderFactory.<init>(SegmentLoaderFactory.java:41)
  while locating org.apache.druid.indexing.common.SegmentLoaderFactory

1 error
 at [Source: HttpInputOverHTTP@3eff14c3[c=1623,q=0,[0]=null,s=STREAM]; line: 47, column: 13] (through reference chain: org.apache.druid.indexing.common.task.IndexTask["spec"]->org.apache.druid.indexing.common.task.IndexIngestionSpec["ioConfig"]->org.apache.druid.indexing.common.task.IndexIOConfig["firehose"])

glasser · 2019-02-19T09:42:37Z

OK, I had a better idea — it seemed like not too hard to improve SegmentLoaderFactory to be injectible without needing to inject SegmentLoaderConfig. Hopefully this works; I stayed up too late and don't want to wait for tests to run :)

glasser · 2019-02-19T15:06:19Z

Ok great, that worked... But I don't understand how to interpret the Team City report. Looks like tens of thousands of errors in files I didn't touch...

glasser · 2019-02-19T16:21:15Z

OK, looks like it somehow split my 4-commit PR into two changes (because I pushed twice?) The first adds 1 error and removes 2813. The second adds 2812 errors and removes 1. That... seems like net negative? (That one new added error is definitely a problem in an earlier part of my PR that is fixed on HEAD.)

Did it get confused because I rebased?

jihoonson · 2019-02-20T21:31:52Z

@glasser thanks for raising a new PR. Sadly, TeamCity sometimes reports something weird. I restarted it.

jihoonson

@glasser thanks! I think this PR is definitely better!

jihoonson · 2019-02-20T22:00:48Z

I don't think it's necessary after this PR, but would be worth to double check.

OK. Do you think just removing this statement and running integration tests would be enough to double check?

Yeah, I think it would be enough.

jihoonson · 2019-02-20T22:25:52Z

There's a difference between using SegmentListUsedAction and directly calling this API: users can configure the retry policy on failure for all remote task actions including SegmentListUsedAction. (I'm not sure this is intended, but the actual max number of retries for remote task actions is RetryPolicy.maxNumRetries * DruidLeaderClient.MAX_RETRIES).

Can we do the same thing for the coordinator client to provide the same experience?

OK, what are you proposing exactly?

(a) Should there be a separate retry policy config bound elsewhere, or should this reuse druid.peon.taskActionClient.retry
(b) Should the retry be performed inside this method here, or in the caller?

(a) I think we should reuse the same configuration name for backward compatibility.
(b) I think it's fine to retry in this method.

Hmm, the whole RetryPolicy set of classes is defined in indexing-service, so I think I'll ignore your suggestion on (b) and do it in IngestSegmentFirehoseFactory.

I'm fine by moving CoordinatorClient to indexing-service along with CoordinatorBasedSegmentHandoffNotifier, CoordinatorBasedSegmentHandoffNotifierConfig, and CoordinatorBasedSegmentHandoffNotifierFactory , but it's up to you.

CoordinatorBasedSegmentHandoffNotifierFactory is used by RealtimeModule which seems to otherwise use very little (though nonzero) from indexing-service. I think doing this move feels a little intense to me and I'd like to leave it alone if that's OK.

jihoonson · 2019-02-20T22:28:28Z

What do you mean by encoding url here?

Hmm, it's confusing that IntelliJ reformatted this part, which is unchanged other than my comment. I do think my settings are based on the Druid configuration...

In my new method below (getDatabaseSegmentDataSourceSegments) it was necessary for me to write StringUtils.urlEncode(dataSource) in order to get the integration tests (which helpfully put a bunch of non-ASCII characters in the data source name) to pass.

isHandOffComplete above also does that. But this method does not. So that makes me think that this method has a bug (unrelated to the work in this PR).

Oh, yeah dataSource should be encoded. Thanks for catching it!

glasser · 2019-02-21T05:35:32Z

OK, responded to your comments and Travis is passing. Commits are intended to be squashed; the PR description has been updated to be a good commit description.

jihoonson

LGTM. I left a couple of trivial comments. Thanks @glasser!

jihoonson · 2019-02-22T19:56:28Z

This looks good to me, but I feel like we can use RetryUtils.retry() to remove duplicate codes even though the retry logic is slightly different. It's up to you.

It seems like the logic in RetryUtils is somewhat different and not configurable. If we want to keep the compatibility I'd rather keep it this way. A nice little project might be to move RetryPolicy and friends into core along with RetryUtils and then have a version of RetryUtils.retry that takes a RetryPolicyConfig or something — actually maybe just dropping RetryPolicy and RetryPolicyFactory entirely? But I feel like this PR refactors enough already.

I agree. Integrating RetryPolicy and RetryUtils sounds good, but should be done in a separate PR.

jihoonson · 2019-02-22T19:57:25Z

I restarted teamcity.

glasser · 2019-02-22T21:46:36Z

Yay, all checks passed!

jihoonson · 2019-02-22T21:52:17Z

Nice! I'll merge this PR tonight unless there's any other opinion.

IndexTask had special-cased code to properly send a TaskToolbox to a IngestSegmentFirehoseFactory that's nested inside a CombiningFirehoseFactory, but ParallelIndexSubTask didn't. This change refactors IngestSegmentFirehoseFactory so that it doesn't need a TaskToolbox; it instead gets a CoordinatorClient and a SegmentLoaderFactory directly injected into it. This also refactors SegmentLoaderFactory so it doesn't depend on an injectable SegmentLoaderConfig, since its only method always replaces the preconfigured SegmentLoaderConfig anyway. This makes it possible to use SegmentLoaderFactory without setting druid.segmentCaches.locations to some dummy value. Another goal of this PR is to make it possible for IngestSegmentFirehoseFactory to list data segments outside of connect() --- specifically, to make it a FiniteFirehoseFactory which can query the coordinator in order to calculate its splits. See #7048. This also adds missing datasource name URL-encoding to an API used by CoordinatorBasedSegmentHandoffNotifier.

glasser · 2019-02-23T20:48:15Z

All I did is squash it and Team City got sad :(

jihoonson · 2019-02-24T01:02:49Z

We usually squash commits when the PR is merged (please check https://github.com/apache/incubator-druid/blob/master/CONTRIBUTING.md#if-your-pull-request-shows-conflicts-with-master).

I'm going to merge this PR now. Thanks @glasser!

glasser marked this pull request as ready for review February 19, 2019 07:30

glasser mentioned this pull request Feb 19, 2019

[do not merge: see #7089 instead] ParallelIndexSubTask: support ingestSegment in delegating factories #7063

Closed

jihoonson reviewed Feb 20, 2019

View reviewed changes

This was referenced Feb 20, 2019

Make IngestSegmentFirehoseFactory splittable for parallel ingestion #7048

Merged

new feature: allows user to specify timeZone in TimestampSpec #6365

Closed

jihoonson approved these changes Feb 22, 2019

View reviewed changes

jihoonson merged commit 1c2753a into apache:master Feb 24, 2019

jihoonson added this to the 0.15.0 milestone May 16, 2019

Conversation

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

Uh oh!

Uh oh!

glasser commented Feb 19, 2019

Uh oh!

glasser commented Feb 19, 2019

Uh oh!

glasser commented Feb 19, 2019

Uh oh!

glasser commented Feb 19, 2019

Uh oh!

jihoonson commented Feb 20, 2019

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

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!

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

Uh oh!

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!

jihoonson commented Feb 22, 2019

Uh oh!

glasser commented Feb 22, 2019

Uh oh!

jihoonson commented Feb 22, 2019

Uh oh!

glasser commented Feb 23, 2019

Uh oh!

jihoonson commented Feb 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

glasser commented Feb 19, 2019 •

edited

Loading

glasser commented Feb 21, 2019 •

edited

Loading