Add group_id to the sys.tasks table by surekhasaharan · Pull Request #8304 · apache/druid

surekhasaharan · 2019-08-15T00:21:37Z

Description

This PR adds a group_id column to sys.tasks table. The tasks API of OverlordResource will now show the group_id of tasks.
Added a groupId field to TaskInfo and TaskStatusPlus. The groupId is present in the Task object, now it's exposed via the API's.

This PR has:

been self-reviewed.
added documentation for new or modified features or behaviors.
added unit tests or modified existing tests to cover new code paths.
been tested in a test Druid cluster.

vogievetsky · 2019-08-16T23:31:57Z

  {
    return "TaskStatusPlus{" +
           "id='" + id + '\'' +
+           "groupId='" + groupId + '\'' +


should this have a , to match the rest of the pattern?

oops, yes, i'll give you that comma !

jihoonson

@surekhasaharan thank you for adding groupId. Left a comment.

jihoonson · 2019-08-21T00:32:54Z

  @Nullable
  private final EntryType task;
+  @Nullable
+  private final String groupId;


This information is duplicate since it's already in task. The caller can get groupId by calling getTask().getGroupId().

I see, thank you, removed groupId from here.

ccaominh · 2019-08-21T17:59:10Z

      for (final TaskStuff taskStuff : tasks.values()) {
        if (taskStuff.getStatus().isRunnable()) {
-          TaskInfo t = new TaskInfo(
+          TaskInfo t = new TaskInfo<>(


Thanks for fixing!

ccaominh · 2019-08-21T17:59:21Z

    return ImmutableList.copyOf(
        handler.getCompletedTaskInfo(
-            DateTimes.nowUtc().minus(durationBeforeNow == null ? config.getRecentlyFinishedThreshold() : durationBeforeNow),
+            DateTimes.nowUtc()


Thanks for reformatting!

ccaominh · 2019-08-21T18:20:14Z

    {
      final Future<TaskStatus> taskStatusFuture = tasks.get(taskId);
+      final Optional<Task> task = getTaskStorage().getTask(taskId);
+      final String groupId = task.isPresent() ? task.orNull().getGroupId() : taskId;


The orNull() is misleading since it would allow the subsequent call to getGroupId() to throw an NPE. Change to task.get().getGroupId(), which is also consistent with the preceding task.isPresent().

Also, a comment explaining why the groupId should default to the taskId the caller specified would work well here.

yeah, it should be task.get().getGroupId(), thanks. I think the default value of groupId doesn't matter here, it can be any value, i think null would be okay as well.

ccaominh · 2019-08-21T18:23:02Z

      return new TaskStatusResponse(
          taskId,
          new TaskStatusPlus(
+              taskId,


Why is taskId passed in as the groupId argument? Would using "groupId" or a GROUP_ID named constant work here?

If groupId is changed to parentId, then a named constant NO_PARENT_ID = "" would work well here.

I guess it might be fine to use the taskId as the groupId since the default value of groupId is the taskId. But, since TaskMonitor is used to monitor sub tasks of the parallel indexing task, it could be more realistic to use the groupId of the taskId of the supervisor task.

I used taskId because getTaskStatus only takes a taskId string, instead of a Taskobject, so there was no easy way to get the real groupId without changing the the interface IndexingServiceClient and also as Jihoon mentioned because default is taskId. This particular test works with any value, eg groupId can be any string or null.

In this particular test, the groupId is always "groupId" as seen here. You can use it if you want.

ccaominh · 2019-08-21T18:26:10Z

        new TaskStatusResponse(
            "mytask",
            new TaskStatusPlus(
+                "mytask",


Similar comment here about id and groupId having the same value and changing it if groupId is renamed to parentId

here, it does matter, the value of groupId is null in NoopTask, so it gets set to taskId in AbstractTask and this test checks the TaskStatusResponse later, so this will fail if groupId is not set to taskId

ccaominh · 2019-08-21T18:30:28Z

 |------|-----|-----|
 |task_id|STRING|Unique task identifier|
-|type|STRING|Task type, for example this value is "index" for indexing tasks. See [tasks-overview](../ingestion/tasks.md)|
+|group_id|STRING|Task group ID for this task, for sub tasks, this value is the parent task's ID|


Describe the value if it is not a subtask (looks like the implementation currently uses the value of task_id)

It depends on the task type i guess, for kafka/kinesis the group_id is a combination of type+datasource. For native index tasks, it's same as task_id. The behavior might be different for other task types. Will adjust the docs saying that it depends on the task type.

ccaominh · 2019-08-21T18:40:59Z

  @Nullable
  private final String errorMsg;
+  @Nullable
+  private final String groupId;


Perhaps a more intuitive name for users to understand would be parentId? Renaming it to parentId will also resemble the parent process id concept of most operating systems that many users are familiar with. For non-subtasks, the value would default to the empty string instead of the task id.

This sounds an interesting idea, but I'm not sure if it would make sense with Kafka/Kinesis indexing tasks. The kafka/kinesis supervisor spawns indexing tasks but the supervisor is executed using a thread in the overlord. The tasks have the same groupId which is datasource name where they ingest data into.

Ah, I didn't know there's already a notion of task groups. For consistency, we should continue using "group" then.

ccaominh · 2019-08-21T18:51:39Z

    {
      final Task subTask = (Task) taskObject;
+      try {
+        getTaskStorage().insert(subTask, TaskStatus.running(subTask.getId()));


It looks like the subtask only starts running after subTask.run() is called below. What cleans up the task running status created here if:

The task never starts running (task is not ready, so subTask.run() is not called)

The task starts running but unexpectedly fails during execution

If those scenarios need to be considered, then added tests for them will be useful.

hmm, good point, I set the task status to failure if any of those condition occurs. Not sure how/where to unit test for those cases though.

Ah, this is a test class, so my earlier comment can be disregarded

ccaominh · 2019-08-21T18:55:10Z


    String json = "[{\n"
                  + "\t\"id\": \"index_wikipedia_2018-09-20T22:33:44.911Z\",\n"
+                  + "\t\"groupId\": \"index_wikipedia_2018-09-20T22:33:44.911Z\",\n"


Does it make sense to use a value here that's different from the "id" to confirm the implementation sets the value appropriately (i.e., the implementation does not always have this value equal to "id")?

sure, changed

ccaominh

LGTM 👍

surekhasaharan · 2019-08-22T00:52:04Z

Thanks @ccaominh and @jihoonson for review !

surekhasaharan · 2019-08-22T17:24:34Z

I do not understand why the coveralls reports decreased coverage after this change

ccaominh · 2019-08-22T17:47:58Z

I'm hoping #8374 will fix the issue with the fluctuating coverage reports

jihoonson

LGTM. The coveralls failure looks not legitimate.

Surekha Saharan added 3 commits August 14, 2019 16:20

Add group_id to overlord tasks API and sys.tasks table

fc0b974

adjust test

f044610

modify docs

777b784

surekhasaharan added Area - Metadata Area - SQL Improvement WIP labels Aug 15, 2019

Surekha Saharan added 5 commits August 15, 2019 15:49

Make groupId nullable

70aa55b

Merge branch 'master' of github.com:druid-io/druid into task-group-id

31deac6

fix integration test

cc1b654

Merge branch 'master' of github.com:druid-io/druid into task-group-id

2e0b234

Merge branch 'master' of github.com:druid-io/druid into task-group-id

2c7b366

surekhasaharan removed the WIP label Aug 16, 2019

vogievetsky reviewed Aug 16, 2019

View reviewed changes

Surekha Saharan added 2 commits August 19, 2019 10:50

fix toString

5228f38

Merge branch 'master' of github.com:druid-io/druid into task-group-id

591e9db

jihoonson reviewed Aug 21, 2019

View reviewed changes

Surekha Saharan added 2 commits August 20, 2019 23:50

Remove groupId from TaskInfo

5238706

Merge branch 'master' of github.com:druid-io/druid into task-group-id

8a920ec

ccaominh reviewed Aug 21, 2019

View reviewed changes

Surekha Saharan added 4 commits August 21, 2019 13:20

Merge branch 'master' of github.com:druid-io/druid into task-group-id

50d5d9f

Modify docs and tests

175b1fb

Merge branch 'master' of github.com:druid-io/druid into task-group-id

229c5af

modify TaskMonitorTest

f369a92

ccaominh approved these changes Aug 22, 2019

View reviewed changes

Merge branch 'master' of github.com:druid-io/druid into task-group-id

de1d6be

jihoonson approved these changes Aug 22, 2019

View reviewed changes

surekhasaharan merged commit cf2a2dd into apache:master Aug 22, 2019

clintropolis added this to the 0.16.0 milestone Aug 23, 2019

Conversation

surekhasaharan commented Aug 15, 2019

Description

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!

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!

ccaominh left a comment

Choose a reason for hiding this comment

Uh oh!

surekhasaharan commented Aug 22, 2019

Uh oh!

surekhasaharan commented Aug 22, 2019

Uh oh!

ccaominh commented Aug 22, 2019

Uh oh!

jihoonson left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants