Add ability to provide taskResource for IndexTask.

[potto007]

I had the need to specify requiredCapacity for ingestSegment IndexTask. I added another overloaded constructor to the AbstractFixedIntervalTask class to handle an incoming taskResource attribute. I overloaded the IndexTask constructor to support all internal implementations of IndexTask. I added a new @JsonProperty for taskResource to the primary constructor, with an inline if to handle the case when someone does not use the resource JSON node. I am using this on some of my systems, and I also added a unit test to verify that this works.

[nishantmonu51]

there is already a constr, which takes (id,groupId,resource, dataSource,interval)
Can we use that in indexTask, instead of adding a new one ?

[potto007]

Good point, Nishant. I was hyper-focused on getting access to the "resource" node. 😀 It does make more sense to use the existing constructor. I can just modify IndexTask to do something similar to RealTimeIndexTask for the groupId...

[potto007]

ie:

    super(
        // _not_ the version, just something uniqueish
        makeId(id, ingestionSchema),
        String.format("index_%s", makeDataSource(ingestionSchema)),
        taskResource == null ? new TaskResource(makeId(id, ingestionSchema), 1) : taskResource,
        makeDataSource(ingestionSchema),
        makeInterval(ingestionSchema)
    );

[potto007]

Good point made by Nishant. I was hyper-focused on getting access to the "resource" node. 😀 It does make more sense to use the existing constructor. I've force-pushed a new submission which leaves the abstract unchanged and does the needful from IndexTask.

[nishantmonu51]

👍

[drcrallen]

Is this constructor just for unit tests?

[potto007]

Hmm, apparently so - I did a search for implementers and thought I had hits, but it was late. It is, indeed, only tests that directly instantiate IndexTask. I can update the implementations in IndexTaskTest, TaskSerdeTest, and TaskLifecycleTest and eliminate the constr overload.

[drcrallen]

Or you can just comment that its only used in unit tests. Whichever makes it easier to read.

[potto007]

I've added a comment:

  /**
   * This overloaded constructor without @JsonCreator exists to allow for
   * unit tests simulating a call to IndexTask without taskResource.
   */

[fjy]

@potto007 you'll also need to sign the CLA here:
http://druid.io/community/cla.html

[drcrallen]

@fjy It's there

[drcrallen]

These three makeId's are not guaranteed to fire in the same millisecond. You'll need to orchestrate it such that makeId fires once, and that value is re-used.

[potto007]

So I've gone back to the way I initially had it, with the taskResource logic pushed up into AbstractFixedIntervalTask - doing so allows for the passed id value to be reused for groupId and for generating a new taskResource if passed as null.

[potto007]

So I've gone back to the way I initially had it, with the taskResource logic pushed up into AbstractFixedIntervalTask - doing so allows for the passed id value to be reused for groupId and for generating a new taskResource if passed as null.

[gianm]

This variable is not used.

[potto007]

Crap - that got left over from something else I was doing. Must have wound up in there with the rebasing and force pushing. Sorry, will remove.

[gianm]

Closed and reopened for travis.

This looks good to me modulo the two comments I had in the diff, and assuming the travis build comes back ok. @potto007 I can make those changes when merging the PR if that makes life easier for you. Please let me know.

[potto007]

If you don't mind, that works for me. Otherwise, I can get to it within the hour.

[gianm]

@potto007 sure, I don't mind. The new PR is #1654, and you'll still get credit for the actual commit.