suneet-s commented on code in PR #15311:
URL: https://github.com/apache/druid/pull/15311#discussion_r1381740888
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/actions/UpdateStatusAction.java:
##########
@@ -34,13 +34,24 @@ public class UpdateStatusAction implements TaskAction<Void>
{
@JsonIgnore
private final String status;
+ @JsonIgnore
+ private final TaskStatus statusFull;
+
+ public UpdateStatusAction(
Review Comment:
Should this be marked as deprecated so it can be removed in a future
version. I think it's still being used in some unit tests.
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/actions/UpdateStatusAction.java:
##########
@@ -82,6 +98,7 @@ public String toString()
{
return "UpdateStatusAction{" +
"status=" + status +
+ "statusFull=" + statusFull +
Review Comment:
```suggestion
", statusFull=" + statusFull +
```
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/AbstractTask.java:
##########
@@ -196,11 +197,11 @@ public void cleanUp(TaskToolbox toolbox, TaskStatus
taskStatus) throws Exception
return;
}
+ TaskStatus taskStatusToReport = taskStatus == null
+ ? TaskStatus.failure(id, "Task failed to run")
+ : taskStatus;
// report back to the overlord
- UpdateStatusAction status = new UpdateStatusAction("successful");
- if (taskStatus.isFailure()) {
- status = new UpdateStatusAction("failure");
- }
+ UpdateStatusAction status = new UpdateStatusAction("", taskStatusToReport);
Review Comment:
Why an empty string and not null?
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/actions/UpdateStatusAction.java:
##########
@@ -95,12 +112,12 @@ public boolean equals(Object o)
return false;
}
UpdateStatusAction that = (UpdateStatusAction) o;
- return Objects.equals(status, that.status);
+ return Objects.equals(status, that.status) && Objects.equals(statusFull,
that.statusFull);
}
@Override
public int hashCode()
{
- return Objects.hash(status);
+ return Objects.hash(status, statusFull);
}
Review Comment:
Could you add an EqualsVerifierTest for this class please.
```
EqualsVerifier.forClass(UpdateStatusAction.class).verify();
```
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/actions/UpdateStatusAction.java:
##########
@@ -34,13 +34,24 @@ public class UpdateStatusAction implements TaskAction<Void>
{
@JsonIgnore
private final String status;
+ @JsonIgnore
+ private final TaskStatus statusFull;
+
+ public UpdateStatusAction(
+ String status
+ )
+ {
+ this(status, null);
+ }
@JsonCreator
public UpdateStatusAction(
- @JsonProperty("status") String status
+ @JsonProperty("status") String status,
+ @JsonProperty("statusFull") TaskStatus statusFull
)
{
this.status = status;
+ this.statusFull = statusFull;
Review Comment:
If `status` is deprecated, and callers are expected to use `statusFull` can
we add validation in the constructor to make sure only 1 field is set.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
