kfaraz commented on code in PR #18448:
URL: https://github.com/apache/druid/pull/18448#discussion_r2309322441
##########
processing/src/main/java/org/apache/druid/indexer/TaskInfo.java:
##########
@@ -76,4 +76,14 @@ public EntryType getTask()
{
return task;
}
+
+ /**
+ * Returns a copy of this TaskInfo object with a new StatusType
+ * @param newStatus
+ * @return a new TaskInfo
+ */
+ public TaskInfo<EntryType, StatusType> withNewStatus(StatusType newStatus)
Review Comment:
Nit: I think the `new` prefix can be omitted.
```suggestion
public TaskInfo<EntryType, StatusType> withStatus(StatusType status)
```
##########
processing/src/main/java/org/apache/druid/indexer/TaskInfo.java:
##########
@@ -76,4 +76,14 @@ public EntryType getTask()
{
return task;
}
+
+ /**
+ * Returns a copy of this TaskInfo object with a new StatusType
+ * @param newStatus
+ * @return a new TaskInfo
Review Comment:
```suggestion
* Returns a copy of this TaskInfo object with the given status.
```
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskQueue.java:
##########
@@ -1140,15 +1168,15 @@ private void removeTaskLock(Task task)
*/
static class TaskEntry
{
- private final Task task;
+ private TaskInfo<Task, TaskStatus> taskInfo;
private DateTime lastUpdatedTime;
private ListenableFuture<TaskStatus> future = null;
private boolean isComplete = false;
- TaskEntry(Task task)
+ TaskEntry(TaskInfo<Task, TaskStatus> taskInfo)
{
- this.task = task;
Review Comment:
We could have retained this field to simplify the patch since the `Task` is
going to be final anyway.
Only the status can change:
```
this.task = taskInfo.getTask()
```
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskQueue.java:
##########
@@ -1020,24 +1027,45 @@ public Optional<Task> getActiveTask(String id)
}
/**
- * List of all active and completed tasks currently being managed by this
- * TaskQueue.
+ * Polls {@link #activeTasks} for the task with the corresponding {@code
taskId}
+ * @param taskId
+ * @return an optional TaskInfo
Review Comment:
```suggestion
* Gets the {@link TaskInfo} for the given {@code taskId} from {@link
#activeTasks} if present,
* otherwise returns an empty optional.
```
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskQueue.java:
##########
@@ -835,26 +844,24 @@ void syncFromStorage()
try {
if (active) {
- final Map<String, Task> newTasks =
- CollectionUtils.toMap(taskStorage.getActiveTasks(), Task::getId,
Function.identity());
- final Map<String, Task> oldTasks =
- CollectionUtils.mapValues(activeTasks, entry -> entry.task);
+ final Map<String, TaskInfo<Task, TaskStatus>> newTasks =
CollectionUtils.toMap(taskStorage.getActiveTaskInfos(), (taskInfo) ->
taskInfo.getTask().getId(), Function.identity());
Review Comment:
Style: Might be nicer to put each arg in a separate line.
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/MetadataTaskStorage.java:
##########
@@ -189,6 +199,15 @@ public List<Task> getActiveTasks()
.collect(Collectors.toList());
}
+ @Override
+ public List<TaskInfo<Task, TaskStatus>> getActiveTaskInfos()
+ {
+ return
handler.getTaskInfos(Collections.singletonMap(TaskLookupType.ACTIVE,
ActiveTaskLookup.getInstance()), null)
Review Comment:
Nit: prefer Map.of as it is more concise.
```suggestion
return handler.getTaskInfos(Map.of(TaskLookupType.ACTIVE,
ActiveTaskLookup.getInstance()), null)
```
##########
indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/OverlordResourceTest.java:
##########
@@ -131,7 +131,7 @@ public void setUp()
provisioningStrategy = EasyMock.createMock(ProvisioningStrategy.class);
authConfig = EasyMock.createMock(AuthConfig.class);
overlord = EasyMock.createStrictMock(DruidOverlord.class);
- taskMaster = EasyMock.createStrictMock(TaskMaster.class);
+ taskMaster = EasyMock.createMock(TaskMaster.class);
Review Comment:
Why not strict anymore?
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskQueue.java:
##########
@@ -835,26 +844,24 @@ void syncFromStorage()
try {
if (active) {
- final Map<String, Task> newTasks =
- CollectionUtils.toMap(taskStorage.getActiveTasks(), Task::getId,
Function.identity());
- final Map<String, Task> oldTasks =
- CollectionUtils.mapValues(activeTasks, entry -> entry.task);
+ final Map<String, TaskInfo<Task, TaskStatus>> newTasks =
CollectionUtils.toMap(taskStorage.getActiveTaskInfos(), (taskInfo) ->
taskInfo.getTask().getId(), Function.identity());
+ final Map<String, TaskInfo<Task, TaskStatus>> oldTasks =
CollectionUtils.mapValues(activeTasks, entry -> entry.taskInfo);
// Identify the tasks that have been added or removed from the storage
- final MapDifference<String, Task> mapDifference =
Maps.difference(oldTasks, newTasks);
- final Collection<Task> addedTasks =
mapDifference.entriesOnlyOnRight().values();
- final Collection<Task> removedTasks =
mapDifference.entriesOnlyOnLeft().values();
+ final MapDifference<String, TaskInfo<Task, TaskStatus>> mapDifference
= Maps.difference(oldTasks, newTasks);
+ final Collection<TaskInfo<Task, TaskStatus>> addedTasks =
mapDifference.entriesOnlyOnRight().values();
+ final Collection<TaskInfo<Task, TaskStatus>> removedTasks =
mapDifference.entriesOnlyOnLeft().values();
// Remove tasks not present in metadata store if their lastUpdatedTime
is before syncStartTime
int numTasksRemoved = 0;
- for (Task task : removedTasks) {
+ for (TaskInfo<Task, TaskStatus> task : removedTasks) {
Review Comment:
Looking at this patch, I wonder if `TaskInfo<Task, TaskStatus>` shouldn't be
a separate class of its own.
Probably named `TaskWithStatus`.
(I am not even sure why this class was generic in the first place. The only
other possible type I see the generic
param taking is `TaskIdentifier`, which could have been its own class
`TaskInfoWithId` or something. But we can address this later).
--
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]
