Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 62 additions & 10 deletions api/src/main/java/org/apache/druid/indexer/TaskStatusPlus.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,18 +22,22 @@
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.google.common.base.Preconditions;
import org.apache.druid.java.util.common.StringUtils;
import org.apache.druid.java.util.common.logger.Logger;
import org.joda.time.DateTime;

import javax.annotation.Nullable;
import java.util.Objects;

public class TaskStatusPlus
{
private static final Logger log = new Logger(TaskStatusPlus.class);

private final String id;
private final String type;
private final DateTime createdTime;
private final DateTime queueInsertionTime;
private final TaskState state;
private final TaskState statusCode;
private final RunnerTaskState runnerTaskState;
private final Long duration;
private final TaskLocation location;
Expand All @@ -42,28 +46,68 @@ public class TaskStatusPlus
@Nullable
private final String errorMsg;

public TaskStatusPlus(
String id,
String type, // nullable for backward compatibility
DateTime createdTime,
DateTime queueInsertionTime,
@Nullable TaskState statusCode,
@Nullable RunnerTaskState runnerStatusCode,
@Nullable Long duration,
TaskLocation location,
@Nullable String dataSource, // nullable for backward compatibility
@Nullable String errorMsg
)
{
this(
id,
type,
createdTime,
queueInsertionTime,
statusCode,
statusCode,
runnerStatusCode,
duration,
location,
dataSource,
errorMsg
);
}


@JsonCreator
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,
@Deprecated @JsonProperty("status") @Nullable TaskState status, // present for backwards compatibility

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added the checks

@JsonProperty("runnerStatusCode") @Nullable RunnerTaskState runnerTaskState,
@JsonProperty("duration") @Nullable Long duration,
@JsonProperty("location") TaskLocation location,
@JsonProperty("dataSource") @Nullable String dataSource, // nullable for backward compatibility
@JsonProperty("errorMsg") @Nullable String errorMsg
)
{
if (state != null && state.isComplete()) {
if (statusCode != null && statusCode.isComplete()) {
Preconditions.checkNotNull(duration, "duration");
}
this.id = Preconditions.checkNotNull(id, "id");
this.type = type;
this.createdTime = Preconditions.checkNotNull(createdTime, "createdTime");
this.queueInsertionTime = Preconditions.checkNotNull(queueInsertionTime, "queueInsertionTime");
this.state = state;
//checks for deserialization safety
if (statusCode != null && status == null) {
this.statusCode = statusCode;
} else if (statusCode == null && status != null) {
this.statusCode = status;
} else {
if (statusCode != null && status != null && statusCode != status) {
throw new RuntimeException(StringUtils.format("statusCode[%s] and status[%s] must match", statusCode, status));
}
this.statusCode = statusCode;
}
this.runnerTaskState = runnerTaskState;
this.duration = duration;
this.location = Preconditions.checkNotNull(location, "location");
Expand Down Expand Up @@ -98,14 +142,22 @@ public DateTime getQueueInsertionTime()

@Nullable
@JsonProperty("statusCode")
public TaskState getState()
public TaskState getStatusCode()
{
return statusCode;
}

@Deprecated
@Nullable
@JsonProperty("status")
public TaskState getStatus()
{
return state;
return statusCode;
}

@Nullable
@JsonProperty("runnerStatusCode")
public RunnerTaskState getRunnerTaskState()
public RunnerTaskState getRunnerStatusCode()
{
return runnerTaskState;
}
Expand Down Expand Up @@ -150,7 +202,7 @@ public boolean equals(Object o)
Objects.equals(getType(), that.getType()) &&
Objects.equals(getCreatedTime(), that.getCreatedTime()) &&
Objects.equals(getQueueInsertionTime(), that.getQueueInsertionTime()) &&
getState() == that.getState() &&
getStatusCode() == that.getStatusCode() &&
Objects.equals(getDuration(), that.getDuration()) &&
Objects.equals(getLocation(), that.getLocation()) &&
Objects.equals(getDataSource(), that.getDataSource()) &&
Expand All @@ -165,7 +217,7 @@ public int hashCode()
getType(),
getCreatedTime(),
getQueueInsertionTime(),
getState(),
getStatusCode(),
getDuration(),
getLocation(),
getDataSource(),
Expand All @@ -181,7 +233,7 @@ public String toString()
", type='" + type + '\'' +
", createdTime=" + createdTime +
", queueInsertionTime=" + queueInsertionTime +
", state=" + state +
", statusCode=" + statusCode +
", duration=" + duration +
", location=" + location +
", dataSource='" + dataSource + '\'' +
Expand Down
39 changes: 39 additions & 0 deletions api/src/test/java/org/apache/druid/indexer/TaskStatusPlusTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,45 @@ public void testSerde() throws IOException
Assert.assertEquals(status, mapper.readValue(json, TaskStatusPlus.class));
}

@Test
public void testJsonAttributes() throws IOException

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!!

{
final ObjectMapper mapper = new ObjectMapper();
mapper.registerModule(
new SimpleModule()
.addDeserializer(DateTime.class, new DateTimeDeserializer())
.addSerializer(DateTime.class, ToStringSerializer.instance)
);
final String json = "{\n"
+ "\"id\": \"testId\",\n"
+ "\"type\": \"testType\",\n"
+ "\"createdTime\": \"2018-09-17T06:35:17.392Z\",\n"
+ "\"queueInsertionTime\": \"2018-09-17T06:35:17.392Z\",\n"
+ "\"statusCode\": \"RUNNING\",\n"
+ "\"status\": \"RUNNING\",\n"
+ "\"runnerStatusCode\": \"RUNNING\",\n"
+ "\"duration\": 1000,\n"
+ "\"location\": {\n"
+ "\"host\": \"testHost\",\n"
+ "\"port\": 1010,\n"
+ "\"tlsPort\": -1\n"
+ "},\n"
+ "\"dataSource\": \"ds_test\",\n"
+ "\"errorMsg\": null\n"
+ "}";
TaskStatusPlus taskStatusPlus = mapper.readValue(json, TaskStatusPlus.class);
Assert.assertNotNull(taskStatusPlus);
Assert.assertNotNull(taskStatusPlus.getStatusCode());
Assert.assertTrue(taskStatusPlus.getStatusCode().isRunnable());
Assert.assertNotNull(taskStatusPlus.getRunnerStatusCode());

String serialized = mapper.writeValueAsString(taskStatusPlus);

Assert.assertTrue(serialized.contains("\"status\":"));
Assert.assertTrue(serialized.contains("\"statusCode\":"));
Assert.assertTrue(serialized.contains("\"runnerStatusCode\":"));
}

// Copied from org.apache.druid.jackson.JodaStuff
private static class DateTimeDeserializer extends StdDeserializer<DateTime>
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,6 @@ public Optional<TaskStatus> getTaskStatus(String id)
final TaskStatusResponse response = client.getTaskStatus(id);
return response == null ?
Optional.absent() :
Optional.of(TaskStatus.fromCode(id, response.getStatus().getState()));
Optional.of(TaskStatus.fromCode(id, response.getStatus().getStatusCode()));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,10 @@ class TaskHistory<T extends Task>
{
attemptHistory.forEach(status -> {
Preconditions.checkState(
status.getState() == TaskState.SUCCESS || status.getState() == TaskState.FAILED,
status.getStatusCode() == TaskState.SUCCESS || status.getStatusCode() == TaskState.FAILED,
"Complete tasks should be recorded, but the state of task[%s] is [%s]",
status.getId(),
status.getState()
status.getStatusCode()
);
});
this.spec = spec;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ public void start(long taskStatusCheckingPeriod)
final TaskStatusResponse taskStatusResponse = indexingServiceClient.getTaskStatus(taskId);
final TaskStatusPlus taskStatus = taskStatusResponse.getStatus();
if (taskStatus != null) {
switch (Preconditions.checkNotNull(taskStatus.getState(), "taskState")) {
switch (Preconditions.checkNotNull(taskStatus.getStatusCode(), "taskState")) {
case SUCCESS:
incrementNumSucceededTasks();

Expand Down Expand Up @@ -152,7 +152,7 @@ public void start(long taskStatusCheckingPeriod)
monitorEntry.updateStatus(taskStatus);
break;
default:
throw new ISE("Unknown taskStatus[%s] for task[%s[", taskStatus.getState(), taskId);
throw new ISE("Unknown taskStatus[%s] for task[%s[", taskStatus.getStatusCode(), taskId);
}
}
}
Expand Down Expand Up @@ -459,7 +459,7 @@ SubTaskSpec<T> getSpec()

TaskState getLastState()
{
return lastStatus == null ? TaskState.FAILED : lastStatus.getState();
return lastStatus == null ? TaskState.FAILED : lastStatus.getStatusCode();
}

@Nullable
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ private void checkState(
.filter(entry -> {
final TaskStatusPlus currentStatus = entry.getValue().getCurrentStatus();
return currentStatus != null &&
(currentStatus.getState() == TaskState.SUCCESS || currentStatus.getState() == TaskState.FAILED);
(currentStatus.getStatusCode() == TaskState.SUCCESS || currentStatus.getStatusCode() == TaskState.FAILED);
})
.map(Entry::getKey)
.findFirst()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ public void testBasic() throws InterruptedException, ExecutionException, Timeout
Assert.assertEquals("supervisorId", result.getSpec().getSupervisorTaskId());
Assert.assertEquals("specId" + i, result.getSpec().getId());
Assert.assertNotNull(result.getLastStatus());
Assert.assertEquals(TaskState.SUCCESS, result.getLastStatus().getState());
Assert.assertEquals(TaskState.SUCCESS, result.getLastStatus().getStatusCode());
Assert.assertEquals(TaskState.SUCCESS, result.getLastState());
}
}
Expand All @@ -113,7 +113,7 @@ public void testRetry() throws InterruptedException, ExecutionException, Timeout
Assert.assertEquals("specId" + i, result.getSpec().getId());

Assert.assertNotNull(result.getLastStatus());
Assert.assertEquals(TaskState.SUCCESS, result.getLastStatus().getState());
Assert.assertEquals(TaskState.SUCCESS, result.getLastStatus().getStatusCode());
Assert.assertEquals(TaskState.SUCCESS, result.getLastState());

final TaskHistory<TestTask> taskHistory = monitor.getCompleteSubTaskSpecHistory(specs.get(i).getId());
Expand All @@ -122,8 +122,8 @@ public void testRetry() throws InterruptedException, ExecutionException, Timeout
final List<TaskStatusPlus> attemptHistory = taskHistory.getAttemptHistory();
Assert.assertNotNull(attemptHistory);
Assert.assertEquals(3, attemptHistory.size());
Assert.assertEquals(TaskState.FAILED, attemptHistory.get(0).getState());
Assert.assertEquals(TaskState.FAILED, attemptHistory.get(1).getState());
Assert.assertEquals(TaskState.FAILED, attemptHistory.get(0).getStatusCode());
Assert.assertEquals(TaskState.FAILED, attemptHistory.get(1).getStatusCode());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -881,6 +881,8 @@ public void testGetTaskStatus() throws Exception
TestHelper.makeJsonMapper().writeValueAsString(response1.getEntity()),
TaskStatusResponse.class
);
TaskStatusPlus tsp = taskStatusResponse1.getStatus();
Assert.assertEquals(tsp.getStatusCode(), tsp.getStatus());
Assert.assertEquals(
new TaskStatusResponse(
"mytask",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ public void testOverlordRun() throws Exception
Assert.assertEquals(taskId_0, ((TaskStatusResponse) response.getEntity()).getTask());
Assert.assertEquals(
TaskStatus.running(taskId_0).getStatusCode(),
((TaskStatusResponse) response.getEntity()).getStatus().getState()
((TaskStatusResponse) response.getEntity()).getStatus().getStatusCode()
);

// Simulate completion of task_0
Expand Down Expand Up @@ -296,7 +296,7 @@ private void waitForTaskStatus(String taskId, TaskState status) throws Interrupt
{
while (true) {
Response response = overlordResource.getTaskStatus(taskId);
if (status.equals(((TaskStatusResponse) response.getEntity()).getStatus().getState())) {
if (status.equals(((TaskStatusResponse) response.getEntity()).getStatus().getStatusCode())) {
break;
}
Thread.sleep(10);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ public TaskState getTaskStatus(String taskID)
{
}
);
return taskStatusResponse.getStatus().getState();
return taskStatusResponse.getStatus().getStatusCode();
}
catch (Exception e) {
throw Throwables.propagate(e);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import org.apache.druid.indexer.TaskStatus;
import org.apache.druid.indexer.TaskState;
import org.joda.time.DateTime;

public class TaskResponseObject
Expand All @@ -30,14 +30,14 @@ public class TaskResponseObject
private final String id;
private final DateTime createdTime;
private final DateTime queueInsertionTime;
private final TaskStatus status;
private final TaskState status;

@JsonCreator
private TaskResponseObject(
@JsonProperty("id") String id,
@JsonProperty("createdTime") DateTime createdTime,
@JsonProperty("queueInsertionTime") DateTime queueInsertionTime,
@JsonProperty("status") TaskStatus status
@JsonProperty("status") TaskState status
)
{
this.id = id;
Expand Down Expand Up @@ -65,7 +65,7 @@ public DateTime getQueueInsertionTime()
}

@SuppressWarnings("unused") // Used by Jackson serialization?
public TaskStatus getStatus()
public TaskState getStatus()
{
return status;
}
Expand Down