make 0.13 tasks API backwards compatible with 0.12 (#6333) by surekhasaharan · Pull Request #6334 · apache/druid

surekhasaharan · 2018-09-15T21:21:41Z

/druid/indexer/v1/task/ID/status API in 0.12 returns

{
	"task": "index_wikipedia_2018-09-17T20:48:21.335Z",
	"status": {
		"id": "index_wikipedia_2018-09-17T20:48:21.335Z",
		"status": "RUNNING",
		"duration": -1
	}
}

In master it got changed to

{
	"task": "index_test1_2018-09-17T20:28:55.785Z",
	"status": {
		"id": "index_test1_2018-09-17T20:28:55.785Z",
		"type": "index",
		"createdTime": "2018-09-17T20:28:55.786Z",
		"queueInsertionTime": "1970-01-01T00:00:00.000Z",
		"statusCode": "FAILED",
		"runnerStatusCode": "WAITING",
		"duration": 32373,
		"location": {
			"host": null,
			"port": -1,
			"tlsPort": -1
		},
		"dataSource": "test1",
		"errorMsg": null
	}
}

which breaks the backwards compatibility, so this PR would add both status and statusCode to the response, for example

{
	"task": "index_test1_2018-09-17T20:28:55.785Z",
	"status": {
		"id": "index_test1_2018-09-17T20:28:55.785Z",
		"type": "index",
		"createdTime": "2018-09-17T20:28:55.786Z",
		"queueInsertionTime": "1970-01-01T00:00:00.000Z",
		"statusCode": "FAILED",
		"status": "FAILED",
		"runnerStatusCode": "WAITING",
		"duration": 32373,
		"location": {
			"host": null,
			"port": -1,
			"tlsPort": -1
		},
		"dataSource": "test1",
		"errorMsg": null
	}
}

Also changed runnerStatusCode to runnerStatus to keep things consistent

jihoonson · 2018-09-15T22:34:00Z

Thanks @surekhasaharan. Would you please add a unit test for this?

gianm

@surekhasaharan The purpose of fixing #6333 would be to make 0.13 compatible with how previous releases would have behaved. Please double-check that this new behavior is consistent with how 0.12 would have behaved. I think in that release, the task status API /druid/indexer/v1/task/ID/status would return the status code enum under "status" and the completeTasks API would return it under "statusCode". In master, both are "statusCode".

So I am suggesting we should make them inconsistent (with each other) once again, but consistent with how 0.12 behaved, to preserve API compatibility.

It might be a good idea to emit both "status" and "statusCode" so we can make them consistent in the future (by deprecating one?).

Revert to statusCode and runnerStatusCode

gianm · 2018-09-18T21:18:41Z

@surekhasaharan the integration test errors look legitimate, could you please check them?

jon-wei · 2018-09-20T21:56:32Z

@@ -49,6 +51,7 @@ public TaskStatusPlus(
      @JsonProperty("createdTime") DateTime createdTime,
      @JsonProperty("queueInsertionTime") DateTime queueInsertionTime,
      @JsonProperty("statusCode") @Nullable TaskState state,


Suggest renaming the variable from state to statusCode and getState() to getStatusCode() to match the JsonProperty name

jon-wei · 2018-09-20T22:43:47Z

                  // TaskStorage API doesn't yet allow it.
                  DateTimes.EPOCH,
                  taskInfo.getStatus().getStatusCode(),
+                  taskInfo.getStatus().getStatusCode(),


Let's add a new constructor for TaskStatusPlus without the @JsonCreator annotation that only has "statusCode" which automatically fills in the deprecated "status" parameter with the same value

yes, that's a good suggestion, should we make the existing constructor private as well

Nah that's fine, I don't think it can be private anyway since it's the @JsonCreator

jon-wei · 2018-09-20T22:43:54Z

      @JsonProperty("createdTime") DateTime createdTime,
      @JsonProperty("queueInsertionTime") DateTime queueInsertionTime,
      @JsonProperty("statusCode") @Nullable TaskState state,
+      @Deprecated @JsonProperty("status") @Nullable TaskState status,  // present for backwards compatibility


For deserialization safety, can you add something here like:

if "status" is null and "statusCode" is not null, set both to the value of "statusCode"

If "statusCode" is null and "status" is not null, set both to the value of "status"

if both are non-null, but have different values, log an error

I don't think there should be a situation where new code would be trying to deserialize an old TaskStatusPlus if the rolling upgrade sequence is followed, but I think it'd be good to handle that situation

added the checks

jon-wei · 2018-09-21T18:58:02Z

      @JsonProperty("createdTime") DateTime createdTime,
      @JsonProperty("queueInsertionTime") DateTime queueInsertionTime,
-      @JsonProperty("statusCode") @Nullable TaskState state,
+      @JsonProperty("statusCode") @Nullable TaskState statusCode,


This constructor isn't @JsonCreator so let's get rid of the @JsonProperty annotations here

right, will remove the annotations.

jon-wei · 2018-09-21T18:59:55Z

-    this.status = status;
+    //checks for deserialization safety
+    if (statusCode != null && status == null) {
+      this.state = statusCode;


let's rename state to statusCode

jon-wei · 2018-09-21T19:03:47Z

-    this.state = state;
+    //checks for deserialization safety
+    if (statusCode != null && status == null) {
+      this.state = statusCode;


let's change state to statusCode

new changes

jihoonson · 2018-09-28T00:37:52Z

+      this.status = status;
+    } else {
+      if (statusCode != null && status != null && statusCode != status) {
+        log.error("statusCode[%s] and status[%s] must be same", statusCode, status);


Is there any case that we can allow this mismatch? If so, it would be better log.warn(). Otherwise, it would be better to throw an exception.

I do not think there can be a mismatch, added a RuntimeException.

Looks that this logging is now duplicate with the below exception. I think it's fine to remove it.

jihoonson · 2018-09-28T00:40:07Z

-  private final TaskState state;
+  private final TaskState statusCode;
+  @Deprecated
+  private final TaskState status;


Do we need to keep both variables? Or, we can just keep statusCode only and make getStatus() return statusCode as well?

I think both variables are required with @JsonProperty, else the serialization/deserialization can fail. for eg if status is present in the json, then deserialization fails.

I mean, we can only keep statusCode in this class but the @JsonCreator can have both status and statusCode as its params. Also this class needs to have both getStatus() and getStatusCode() annotated with @JsonProperty. Then, serialization and deserialization would work. In serialization, both status and statusCode would be in the serialized JSON because we have both both getStatus() and getStatusCode() methods. In deserialization, this class can read either status or statusCode (or both) since the constructor accepts both params.

ok, yes that can work

jihoonson · 2018-09-28T00:40:29Z

  }

+  @Test
+  public void testJsonAttributes() throws IOException


jihoonson

@surekhasaharan thanks for the quick fix. Please check my last comment.

jihoonson · 2018-09-30T20:26:22Z

+    } else {
+      if (statusCode != null && status != null && statusCode != status) {
+        log.error("statusCode[%s] and status[%s] must be same", statusCode, status);
+        throw new RuntimeException("statusCode and status must match");


Please add what statusCode and status were in this exception too.

jihoonson · 2018-09-30T20:27:37Z

+      this.status = status;
+    } else {
+      if (statusCode != null && status != null && statusCode != status) {
+        log.error("statusCode[%s] and status[%s] must be same", statusCode, status);


Looks that this logging is now duplicate with the below exception. I think it's fine to remove it.

jihoonson

@surekhasaharan thanks!

…che#6334) * Replace statusCode with status (apache#6333) Also changed runnerStatusCode to runnerStatus to keep things consistent * Add unit test * Add status param to TaskStatusPlus Revert to statusCode and runnerStatusCode * Add additional status member to TaskStatusPlus * Change TaskResponseObject to match overlord's response object * Address PR comments * address comments * Add runtime exception after logging error * Remove (deprecated)status member variable from TaskStatusPlus * Minor change

Replace statusCode with status (apache#6333)

98d131b

Also changed runnerStatusCode to runnerStatus to keep things consistent

fjy added this to the 0.13.0 milestone Sep 16, 2018

Add unit test

e5c99a3

jon-wei previously approved these changes Sep 17, 2018

View reviewed changes

gianm requested changes Sep 17, 2018

View reviewed changes

surekhasaharan changed the title ~~Replace statusCode with status (#6333)~~ make 0.13 tasks API backwards compatible with 0.12 (#6333) Sep 17, 2018

Surekha Saharan added 2 commits September 17, 2018 15:42

Add status param to TaskStatusPlus

5908cb3

Revert to statusCode and runnerStatusCode

Merge branch 'master' of github.com:druid-io/druid into rename-status

6179544

Surekha Saharan added 4 commits September 19, 2018 11:39

Add additional status member to TaskStatusPlus

907837d

Merge branch 'master' of github.com:druid-io/druid into rename-status

36a9892

Change TaskResponseObject to match overlord's response object

4e51de7

Merge branch 'master' of github.com:druid-io/druid into rename-status

c212919

jon-wei reviewed Sep 20, 2018

View reviewed changes

Surekha Saharan added 2 commits September 20, 2018 17:31

Address PR comments

4c700c0

Merge branch 'master' of github.com:druid-io/druid into rename-status

6023fb2

jon-wei reviewed Sep 21, 2018

View reviewed changes

Surekha Saharan added 2 commits September 21, 2018 18:55

address comments

8e0114a

Merge branch 'master' of github.com:druid-io/druid into rename-status

39829ac

jihoonson reviewed Sep 28, 2018

View reviewed changes

gianm assigned jihoonson Sep 28, 2018

Surekha Saharan added 4 commits September 28, 2018 16:48

Merge branch 'master' of github.com:druid-io/druid into rename-status

668ca77

Add runtime exception after logging error

d4dbbad

Merge branch 'master' of github.com:druid-io/druid into rename-status

530285d

Remove (deprecated)status member variable from TaskStatusPlus

a9bb366

jihoonson reviewed Sep 30, 2018

View reviewed changes

Surekha Saharan added 2 commits September 30, 2018 22:14

Merge branch 'master' of github.com:druid-io/druid into rename-status

46d558d

Minor change

08285d4

jihoonson approved these changes Oct 1, 2018

View reviewed changes

gianm approved these changes Oct 1, 2018

View reviewed changes

gianm merged commit 42e5385 into apache:master Oct 1, 2018

surekhasaharan deleted the rename-status branch October 1, 2018 22:40

fjy mentioned this pull request Oct 2, 2018

Tasks are reporting statusCode instead of status #6333

Closed

dclim mentioned this pull request Nov 27, 2018

Druid 0.13.0-incubating release notes #6442

Closed

Conversation

surekhasaharan commented Sep 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jihoonson commented Sep 15, 2018

Uh oh!

gianm left a comment

Choose a reason for hiding this comment

Uh oh!

gianm commented Sep 18, 2018

Uh oh!

jon-wei Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

surekhasaharan commented Sep 15, 2018 •

edited

Loading

jon-wei Sep 20, 2018 •

edited

Loading