Allow task to override ForkingTaskRunner tunings and jvm settings #1604

nishantmonu51 · 2015-08-06T13:13:21Z

Initial PR for discussion.

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

TODO:

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

himanshug · 2015-08-13T03:51:01Z

indexing-service/src/main/java/io/druid/indexing/common/task/AbstractTask.java

+  @Override
+  public <ContextType> ContextType getContextValue(String key)
+  {
+    return context == null ? null : (ContextType) context.get(key);


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

I agree there. Can this just be Object?

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

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 ?

Adding new comments to modified code

himanshug · 2015-08-13T03:52:31Z

looks ok to me overall on first glance

drcrallen · 2015-09-01T18:31:26Z

indexing-service/src/main/java/io/druid/indexing/common/task/Task.java

+
+  public Map<String, Object> getContext();
+
+  public <ContextType> ContextType getContextValue(String key);


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

changing to Object as return type.

nishantmonu51 · 2015-09-02T17:44:11Z

@himanshug @drcrallen handled the review comments.

drcrallen · 2015-09-02T17:50:04Z

indexing-service/src/main/java/io/druid/indexing/common/task/AbstractTask.java

+  public <ContextType> ContextType getContextValue(String key, ContextType defaultValue)
+  {
+    Object retVal = getContextValue(key);
+    return retVal == null ? defaultValue : (ContextType) retVal;


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)

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

drcrallen · 2015-09-02T18:19:38Z

Cool, I'm 👍 after travis

himanshug · 2015-09-02T20:08:44Z

indexing-service/src/test/java/io/druid/indexing/common/task/TaskContextTest.java

+    // Verify Typecaset by passing object of another type as Default value
+    IContext val = noopTask.getContextValue("k1", new Impl2());
+    Assert.assertEquals(contextValue, val);
+  }


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.

removed the method, it was not used anywhere anyways.

override javaOpts fix compilation review comments Add Test for typecast review comments - remove unused method.

himanshug · 2015-09-03T18:29:21Z

LGTM (after Travis)

nishantmonu51 · 2015-09-03T18:52:26Z

bouncing for travis

Allow task to override ForkingTaskRunner tunings and jvm settings

guobingkun · 2015-09-04T15:16:42Z

indexing-service/src/main/java/io/druid/indexing/overlord/ForkingTaskRunner.java

@@ -201,6 +214,19 @@ public TaskStatus call()
                                }
                              }

+                              // Override task specific properties
+                              for (String propName : task.getContext().keySet()) {


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

nishantmonu51 · 2015-09-04T16:10:52Z

@guobingkun thanks for the catch, created #1703.

nishantmonu51 added the Discuss label Aug 6, 2015

himanshug reviewed Aug 13, 2015
View reviewed changes

nishantmonu51 force-pushed the task-jvm-config branch from 4d72094 to b4a269c Compare August 31, 2015 11:49

nishantmonu51 mentioned this pull request Aug 31, 2015

Priority based task locking #1679

Closed

drcrallen reviewed Sep 1, 2015
View reviewed changes

nishantmonu51 force-pushed the task-jvm-config branch from b4a269c to f50874d Compare September 2, 2015 16:24

drcrallen reviewed Sep 2, 2015
View reviewed changes

nishantmonu51 force-pushed the task-jvm-config branch from f50874d to bcdf7a5 Compare September 2, 2015 18:08

himanshug reviewed Sep 2, 2015
View reviewed changes

Add Task Context and ability to override task specific properties

726326a

override javaOpts fix compilation review comments Add Test for typecast review comments - remove unused method.

nishantmonu51 force-pushed the task-jvm-config branch from bcdf7a5 to 726326a Compare September 3, 2015 18:13

nishantmonu51 closed this Sep 3, 2015

nishantmonu51 reopened this Sep 3, 2015

himanshug added a commit that referenced this pull request Sep 3, 2015

Merge pull request #1604 from metamx/task-jvm-config

66a9a33

Allow task to override ForkingTaskRunner tunings and jvm settings

himanshug merged commit 66a9a33 into apache:master Sep 3, 2015

guobingkun reviewed Sep 4, 2015
View reviewed changes

cheddar mentioned this pull request Sep 19, 2016

add context to kafka supervisor for the kafka indexing task #3464

Merged

jihoonson mentioned this pull request Jul 17, 2017

Passing lockTimeout as a parameter for TaskLockbox.lock() #4549

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow task to override ForkingTaskRunner tunings and jvm settings #1604

Allow task to override ForkingTaskRunner tunings and jvm settings #1604

nishantmonu51 commented Aug 6, 2015

himanshug Aug 13, 2015

drcrallen Sep 1, 2015

nishantmonu51 Sep 2, 2015

drcrallen Sep 2, 2015

himanshug commented Aug 13, 2015

drcrallen Sep 1, 2015

nishantmonu51 Sep 2, 2015

nishantmonu51 commented Sep 2, 2015

drcrallen Sep 2, 2015

nishantmonu51 Sep 2, 2015

drcrallen commented Sep 2, 2015

himanshug Sep 2, 2015

nishantmonu51 Sep 3, 2015

himanshug commented Sep 3, 2015

nishantmonu51 commented Sep 3, 2015

guobingkun Sep 4, 2015

nishantmonu51 commented Sep 4, 2015


		public Map<String, Object> getContext();

		public <ContextType> ContextType getContextValue(String key);

Allow task to override ForkingTaskRunner tunings and jvm settings #1604

Allow task to override ForkingTaskRunner tunings and jvm settings #1604

Conversation

nishantmonu51 commented Aug 6, 2015

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

himanshug commented Aug 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nishantmonu51 commented Sep 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drcrallen commented Sep 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himanshug commented Sep 3, 2015

nishantmonu51 commented Sep 3, 2015

Choose a reason for hiding this comment

nishantmonu51 commented Sep 4, 2015