Make supervisor API similar to submit task API by surekhasaharan · Pull Request #8810 · apache/druid

surekhasaharan · 2019-11-02T02:38:34Z

Description

Make submit task and supervisor API's similar in structure. supervisor API accepts both forms.

New form:

{
  "type": "kafka",
  "spec": {
    "dataSchema": { ... },
    "tuningConfig": { ... },
    "ioConfig": { ... }
  }
}

and old form:

{
  "type": "kafka",
   "dataSchema": { ... },
   "tuningConfig": { ... },
   "ioConfig": { ... }
}

The supervisor spec would allow both formats, but the serialization is done to the old format to allow for backwards compatibility.

This PR has:

been self-reviewed.
added unit tests or modified existing tests to cover new code paths.
been tested in a test Druid cluster.

…sk json

…to submit-task-api

lgtm-com · 2019-11-06T01:23:13Z

This pull request introduces 3 alerts when merging c34c5cf into 3b602da - view on LGTM.com

new alerts:

3 for Dereferenced variable may be null

…to submit-task-api

lgtm-com · 2019-11-06T08:02:58Z

This pull request introduces 1 alert when merging 8d0c0a6 into 6f7fbeb - view on LGTM.com

new alerts:

1 for Dereferenced variable may be null

jihoonson · 2019-11-06T23:25:28Z

Please fix LGTM alert.

surekhasaharan · 2019-11-08T21:39:50Z

Please fix LGTM alert.

Yeah, i looked at that, it thinks dataSchema can be null here because of test cases, where null is passed for dataSchema, though those tests pass a valid ingestionSchema, and it's already checked in here. Not sure how to make lgtm ignore that or fix otherwise.

vogievetsky · 2019-11-08T22:17:46Z

I am a strong 👍 on the design (I am biased since I filed the original issue).
This will allow us to take a nasty hack out of the web console.

jihoonson

Yeah, i looked at that, it thinks dataSchema can be null here because of test cases, where null is passed for dataSchema, though those tests pass a valid ingestionSchema, and it's already checked in here. Not sure how to make lgtm ignore that or fix otherwise.

You can fix it by making a non-null IndexIngestionSpec first and then using it in the constructor.

Also, this PR changes only IndexTask. Are you going to change ParallelIndexSupervisorTask in a follow-up pr?

jihoonson · 2019-11-08T22:28:05Z

 {
-  "type" : "index",
-  "spec" : {
+    "type" : "index",


Wrong indentation. Should be 2 spaces.

jihoonson · 2019-11-08T22:29:49Z

+  )
  {
-    return makeGroupId(ingestionSchema.ioConfig.appendToExisting, ingestionSchema.dataSchema.getDataSource());
+    final boolean isValid = (ingestionSchema != null) ^ (dataSchema != null


This check should be in the constructor rather than here since it's a method to make a groupId.

makeGroupId is called from the constructor inside this, so can't add another check before calling this

jihoonson · 2019-11-08T22:30:52Z

+                                                         && ioConfig != null
+                                                         && tuningConfig != null);
+    if (!isValid) {
+      throw new ISE("invalid spec input");


Please make the error message more user friendly. It should say what's missing or what's duplicate.

jihoonson · 2019-11-08T22:31:37Z

+    }
+    if (ingestionSchema == null) {
+      assert (ioConfig != null);
+      assert (dataSchema != null);


Why these asserts? Seems duplicate.

these were added because of lgtm warnings

jihoonson · 2019-11-08T22:32:42Z

@@ -174,6 +191,9 @@ public IndexTask(
      @JsonProperty("id") final String id,
      @JsonProperty("resource") final TaskResource taskResource,
      @JsonProperty("spec") final IndexIngestionSpec ingestionSchema,


Should be @Deprecated.

jihoonson · 2019-11-08T22:35:13Z

            false
        ),
        null,
+        null,


Please add another constructor to IndexTask which is compatible to the old constructor so that you can minimize the change of this PR. The constructor should be @VisibleForTesting and @Deprecated.

added the original constructor back

jihoonson · 2019-11-08T22:51:06Z

+
+    final String json = jsonMapper.writeValueAsString(task);
+
+    Thread.sleep(100); // Just want to run the clock a bit to make sure the task id doesn't change


I don't understand this comment. What does it do?

copy-paste of another test case :). Tried removing the sleep, test case still passes.

jihoonson · 2019-11-08T22:51:52Z

+  @Test(expected = ISE.class)
+  public void testIndexTaskInvalidSpecSerde()
+  {
+    new IndexTask(


This is not a serde test. The test should deserialize from a json string.

moved this to IndexTaskTest

…to submit-task-api

surekhasaharan · 2019-11-13T20:56:33Z

Yeah, i looked at that, it thinks dataSchema can be null here because of test cases, where null is passed for dataSchema, though those tests pass a valid ingestionSchema, and it's already checked in here. Not sure how to make lgtm ignore that or fix otherwise.

You can fix it by making a non-null IndexIngestionSpec first and then using it in the constructor.

Also, this PR changes only IndexTask. Are you going to change ParallelIndexSupervisorTask in a follow-up pr?

Yes, want to change ParallelIndexSupervisorTask as well in this PR, working on it.

…to submit-task-api

surekhasaharan · 2019-11-18T19:55:18Z

Thanks @jihoonson for review and suggestion, I have updated this PR to allow supervisor schema to take in spec (the old format is still valid) similar to index, index_parallel and hadoop submit task APIs

jihoonson · 2019-11-19T02:12:32Z

@surekhasaharan thanks for updating the PR. Would you please merge master so that LGTM can be passed?

…to submit-task-api

jihoonson · 2019-11-19T21:59:01Z


+  private static KafkaSupervisorTuningConfig getTuningConfig(
+      KafkaSupervisorIngestionSpec ingestionSchema,
+      KafkaSupervisorTuningConfig tuningConfig


Please annotate both @Nullable.

jihoonson · 2019-11-19T21:59:42Z

+
  @JsonCreator
  public KafkaSupervisorSpec(
+      @JsonProperty("spec") KafkaSupervisorIngestionSpec ingestionSchema,


Please annotate ingestionSchema, dataSchema, tuningConfig, and ioConfig with @Nullable.

jihoonson · 2019-11-19T22:00:32Z

+  @JsonProperty
+  public DataSchema getDataSchema()
+  {
+    return super.getDataSchema();


Seems unnecessary.

And getDataSchema() should be deprecated.

removed from subclass and deprecated in base class

jihoonson · 2019-11-19T22:06:56Z

+
  @Override
  @JsonProperty
  public KafkaSupervisorTuningConfig getTuningConfig()


This and getIoConfig() should be deprecated now.

jihoonson · 2019-11-19T22:09:12Z

+  @JsonProperty
+  public KafkaSupervisorIngestionSpec getSpec()
+  {
+    return (KafkaSupervisorIngestionSpec) super.getSpec();


@Nullable

Hmm sorry now it's not nullable.

oh, yeah removed

jihoonson · 2019-11-19T22:13:11Z

  protected final ObjectMapper mapper;
  protected final RowIngestionMetersFactory rowIngestionMetersFactory;
+  private final SeekableStreamSupervisorIngestionSpec ingestionSchema;
  private final DataSchema dataSchema;


This class now has duplicate information between ingestionSchema and dataSchema, ioConfig, and tuningConfig. I suggest to remove dataSchema, ioConfig, and tuningConfig and keep only ingestionSchema.

These will still be required in subclasses KafkaSupervisorSpec and KinesisSupervisorSpec for backwards compatibility, so there will be some duplication.

It doesn't have to be duplicate. You can always create ingestionSchema and get things from it.

ok right, removed the extra configs from this class.

…to submit-task-api

jihoonson · 2019-11-20T05:26:55Z

+  @JsonProperty
+  public KafkaSupervisorIngestionSpec getSpec()
+  {
+    return (KafkaSupervisorIngestionSpec) super.getSpec();


Hmm sorry now it's not nullable.

jihoonson · 2019-11-20T05:27:34Z

  }

+  @Override
+  @Nullable


Same here. It's not nullable now.

jihoonson · 2019-11-20T05:30:29Z

Thanks. I left a couple of trivial comments. LGTM after removing wrong nullable annotations.

…to submit-task-api

jihoonson

+1 after CI. Thanks!

* accept spec or dataSchema, tuningConfig, ioConfig while submitting task json * fix test * update docs * lgtm warning * Add original constructor back to IndexTask to minimize changes * fix indentation in docs * Allow spec to be specified in supervisor schema * undo IndexTask spec changes * update docs * Add Nullable and deprecated annotations * remove deprecated configs from SeekableStreamSupervisorSpec * remove nullable annotation

accept spec or dataSchema, tuningConfig, ioConfig while submitting ta…

2d314e3

…sk json

surekhasaharan added the Ease of Use label Nov 2, 2019

surekhasaharan changed the title ~~accept spec or dataSchema, tuningConfig, ioConfig while submitting~~ Make submit task API similar to supervisor API Nov 2, 2019

fix test

bd4c1ce

surekhasaharan added the Area - Batch Ingestion label Nov 2, 2019

Surekha Saharan added 2 commits November 1, 2019 23:13

update docs

ffff56b

Merge branch 'master' of https://github.com/apache/incubator-druid in…

c34c5cf

…to submit-task-api

Surekha Saharan added 2 commits November 5, 2019 22:54

lgtm warning

5cab838

Merge branch 'master' of https://github.com/apache/incubator-druid in…

8d0c0a6

…to submit-task-api

jihoonson added Design Review Release Notes labels Nov 6, 2019

jihoonson reviewed Nov 8, 2019

View reviewed changes

Surekha Saharan added 3 commits November 13, 2019 12:47

Add original constructor back to IndexTask to minimize changes

473a969

Merge branch 'master' of https://github.com/apache/incubator-druid in…

4aaa3f8

…to submit-task-api

fix indentation in docs

47fd2ba

Surekha Saharan added 3 commits November 16, 2019 22:08

Merge branch 'master' of https://github.com/apache/incubator-druid in…

1036301

…to submit-task-api

Allow spec to be specified in supervisor schema

4c59452

Merge branch 'master' of https://github.com/apache/incubator-druid in…

bb22f82

…to submit-task-api

surekhasaharan added the WIP label Nov 18, 2019

Surekha Saharan added 3 commits November 18, 2019 10:31

undo IndexTask spec changes

65d0bf5

update docs

dc4cbed

Merge branch 'master' of https://github.com/apache/incubator-druid in…

cd3ea65

…to submit-task-api

surekhasaharan removed the WIP label Nov 18, 2019

surekhasaharan changed the title ~~Make submit task API similar to supervisor API~~ Make supervisor API similar to submit task API Nov 18, 2019

Surekha Saharan added 2 commits November 18, 2019 21:27

Merge branch 'master' of https://github.com/apache/incubator-druid in…

98c82eb

…to submit-task-api

Merge branch 'master' of https://github.com/apache/incubator-druid in…

397e467

…to submit-task-api

jihoonson reviewed Nov 19, 2019

View reviewed changes

Surekha Saharan added 3 commits November 19, 2019 15:42

Add Nullable and deprecated annotations

9384bce

Merge branch 'master' of https://github.com/apache/incubator-druid in…

da93905

…to submit-task-api

remove deprecated configs from SeekableStreamSupervisorSpec

e70e42f

jihoonson reviewed Nov 20, 2019

View reviewed changes

Surekha Saharan added 2 commits November 19, 2019 22:09

remove nullable annotation

be92a1d

Merge branch 'master' of https://github.com/apache/incubator-druid in…

3bf5601

…to submit-task-api

jihoonson approved these changes Nov 20, 2019

View reviewed changes

jihoonson merged commit d628beb into apache:master Nov 20, 2019

surekhasaharan deleted the submit-task-api branch November 20, 2019 18:43

jon-wei added this to the 0.17.0 milestone Dec 17, 2019

jon-wei mentioned this pull request Dec 28, 2019

0.17.0 release notes #9066

Closed

techdocsmith mentioned this pull request Aug 23, 2021

updates Kafka and Kinesis to use . Fixes some typos and other style i… #11624

Merged


		final String json = jsonMapper.writeValueAsString(task);

		Thread.sleep(100); // Just want to run the clock a bit to make sure the task id doesn't change

Conversation

surekhasaharan commented Nov 2, 2019 • edited by jon-wei Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

lgtm-com Bot commented Nov 6, 2019

Uh oh!

lgtm-com Bot commented Nov 6, 2019

Uh oh!

jihoonson commented Nov 6, 2019

Uh oh!

surekhasaharan commented Nov 8, 2019

Uh oh!

vogievetsky commented Nov 8, 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!

Choose a reason for hiding this comment

Uh oh!

surekhasaharan commented Nov 13, 2019

Uh oh!

surekhasaharan commented Nov 18, 2019

Uh oh!

jihoonson commented Nov 19, 2019

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!

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!

surekhasaharan commented Nov 2, 2019 •

edited by jon-wei

Loading