Allow task to override ForkingTaskRunner tunings and jvm settings

[nishantmonu51]

Initial PR for discussion.

1. Adds a context to tasks which can be used to specify environment configs for tasks (similar to query context)
2. Allow tasks to override fork properties and jvm memory settings.

TODO:

1. Add serde tests for task context
2. UTs to verify forkingTaskRunner overriding properties specified in taskContext.

[himanshug]

not sure if we are gaining anything by using generics in this case?

[drcrallen]

I agree there. Can this just be Object?

or alternatively, have some other way of doing better type enforcing?

[nishantmonu51]

I used the same semantics as used by druid queries.
I like this way so that user of this method need not worry about casting the returned value.
I can change it to Object as return type if you feel strongly about it ?

[drcrallen]

Adding new comments to modified code

[himanshug]

looks ok to me overall on first glance

[drcrallen]

As per prior comment. Suggest simply making Object and make the caller handle proper type checking.

[nishantmonu51]

changing to Object as return type.

[nishantmonu51]

@himanshug @drcrallen handled the review comments.

[drcrallen]

Can you add a unit test for this case:

Have an interface which is expected from the context. But specify an implementation of that interface as the default value.

Does the casting and type detection work correctly? More specifically, in such a case is <ContextType> determined from the return or the parameter?

It may need something like public <ContextType, ContextTypeSub extends ContextType> ContextType getContextValue(String key, ContextTypeSub defaultValue)

[nishantmonu51]

added test.
the type detection is done on return type.

[drcrallen]

Cool, I'm 👍 after travis

[himanshug]

java is funny with generics.
Can you try context = ImmutableMap.<String, Object>of("k1", 10)
then try getting getContextValue("k1","hello")

for me it failed and I believe to make it really work you have to do what @drcrallen suggested. However, at the same time, I don't know if so much complication is really needed here. Is getContextValue(key,default) really essential or we should just remove it.
For me getContext() and getContextValue(key) would already be enough.

[nishantmonu51]

removed the method, it was not used anywhere anyways.

[himanshug]

LGTM (after Travis)

[nishantmonu51]

bouncing for travis

[guobingkun]

@nishantmonu51 I think this will throw NPE if context is not provided.

[nishantmonu51]

@guobingkun thanks for the catch, created #1703.