Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make 0.13 tasks API backwards compatible with 0.12 (#6333) #6334

Merged
merged 18 commits into from Oct 1, 2018

Conversation

Projects
None yet
5 participants
@surekhasaharan
Copy link
Contributor

surekhasaharan commented Sep 15, 2018

/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
	}
}
Replace statusCode with status (#6333)
Also changed runnerStatusCode to runnerStatus to keep things consistent
@jihoonson

This comment has been minimized.

Copy link
Contributor

jihoonson commented Sep 15, 2018

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

@fjy fjy added this to the 0.13.0 milestone Sep 16, 2018

@gianm
Copy link
Contributor

gianm left a comment

@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?).

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

@gianm

This comment has been minimized.

Copy link
Contributor

gianm commented Sep 18, 2018

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

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

This comment has been minimized.

Copy link
@jon-wei

jon-wei Sep 20, 2018

Contributor

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

This comment has been minimized.

Copy link
@surekhasaharan

surekhasaharan Sep 21, 2018

Author Contributor

done

@@ -267,6 +267,7 @@ public Response getTaskStatus(@PathParam("taskid") String taskid)
// TaskStorage API doesn't yet allow it.
DateTimes.EPOCH,
taskInfo.getStatus().getStatusCode(),
taskInfo.getStatus().getStatusCode(),

This comment has been minimized.

Copy link
@jon-wei

jon-wei Sep 20, 2018

Contributor

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

This comment has been minimized.

Copy link
@surekhasaharan

surekhasaharan Sep 21, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@jon-wei

jon-wei Sep 21, 2018

Contributor

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

@@ -49,6 +51,7 @@ public TaskStatusPlus(
@JsonProperty("createdTime") DateTime createdTime,
@JsonProperty("queueInsertionTime") DateTime queueInsertionTime,
@JsonProperty("statusCode") @Nullable TaskState state,
@Deprecated @JsonProperty("status") @Nullable TaskState status, // present for backwards compatibility

This comment has been minimized.

Copy link
@jon-wei

jon-wei Sep 20, 2018

Contributor

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

This comment has been minimized.

Copy link
@surekhasaharan

surekhasaharan Sep 21, 2018

Author Contributor

added the checks

public TaskStatusPlus(
@JsonProperty("id") String id,
@JsonProperty("type") @Nullable String type, // nullable for backward compatibility
@JsonProperty("createdTime") DateTime createdTime,
@JsonProperty("queueInsertionTime") DateTime queueInsertionTime,
@JsonProperty("statusCode") @Nullable TaskState state,
@JsonProperty("statusCode") @Nullable TaskState statusCode,

This comment has been minimized.

Copy link
@jon-wei

jon-wei Sep 21, 2018

Contributor

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

This comment has been minimized.

Copy link
@surekhasaharan

surekhasaharan Sep 22, 2018

Author Contributor

right, will remove the annotations.

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

This comment has been minimized.

Copy link
@jon-wei

jon-wei Sep 21, 2018

Contributor

let's rename state to statusCode

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

This comment has been minimized.

Copy link
@jon-wei

jon-wei Sep 21, 2018

Contributor

let's change state to statusCode

This comment has been minimized.

Copy link
@surekhasaharan

surekhasaharan Sep 22, 2018

Author Contributor

ok

new changes

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

This comment has been minimized.

Copy link
@jihoonson

jihoonson Sep 28, 2018

Contributor

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.

This comment has been minimized.

Copy link
@surekhasaharan

surekhasaharan Sep 29, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@jihoonson

jihoonson Sep 30, 2018

Contributor

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

This comment has been minimized.

Copy link
@surekhasaharan

surekhasaharan Oct 1, 2018

Author Contributor

sure

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

This comment has been minimized.

Copy link
@jihoonson

jihoonson Sep 28, 2018

Contributor

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

This comment has been minimized.

Copy link
@surekhasaharan

surekhasaharan Sep 29, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@jihoonson

jihoonson Sep 29, 2018

Contributor

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.

This comment has been minimized.

Copy link
@surekhasaharan

surekhasaharan Sep 29, 2018

Author Contributor

ok, yes that can work

@@ -61,6 +61,45 @@ public void testSerde() throws IOException
Assert.assertEquals(status, mapper.readValue(json, TaskStatusPlus.class));
}

@Test
public void testJsonAttributes() throws IOException

This comment has been minimized.

Copy link
@jihoonson

jihoonson Sep 28, 2018

Contributor

Thanks!!

@jihoonson
Copy link
Contributor

jihoonson left a comment

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

} 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");

This comment has been minimized.

Copy link
@jihoonson

jihoonson Sep 30, 2018

Contributor

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

This comment has been minimized.

Copy link
@surekhasaharan

surekhasaharan Oct 1, 2018

Author Contributor

okay

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

This comment has been minimized.

Copy link
@jihoonson

jihoonson Sep 30, 2018

Contributor

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

@jihoonson
Copy link
Contributor

jihoonson left a comment

@surekhasaharan thanks!

@gianm

gianm approved these changes Oct 1, 2018

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

2 checks passed

Inspections: pull requests (Druid) TeamCity build finished
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@surekhasaharan surekhasaharan deleted the surekhasaharan:rename-status branch Oct 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.