Add stats to KillUnusedSegments coordinator duty by zachjsh · Pull Request #14782 · apache/druid

zachjsh · 2023-08-08T22:34:06Z

Description

Added the following metrics, which are calculated from the KillUnusedSegments coordinatorDuty

"killTask/availableSlot/count": calculates the number remaining task slots available for auto kill
"killTask/maxSlot/count": calculates the maximum number of tasks available for auto kill
"killTask/task/count": calculates the number of tasks submitted by auto kill.

Release note

NEW: metrics added for auto kill

"killTask/availableSlot/count": calculates the number remaining task slots available for auto kill
"killTask/maxSlot/count": calculates the maximum number of tasks available for auto kill
"killTask/task/count": calculates the number of tasks submitted by auto kill.

This PR has:

jasonk000

looks good

jasonk000 · 2023-08-09T18:12:08Z


  private static final Logger LOG = new Logger(CompactSegments.class);

+  private static final Predicate<TaskStatusPlus> TASK_PREDICATE =


Can we name this COMPACTION_TASK_PREDICATE, or IS_COMPACTION_TASK?

jasonk000 · 2023-08-09T18:19:40Z

+        // the tasks of the unknown taskType as the killTask. This is because it's important to not run
+        // killTasks more than the configured limit at any time which might impact to the ingestion
+        // performance.
+        if (status.getType() == null || (taskPredicate.apply(status))) {


I observe something unclear in the null check here:

First, we assume status is non-null because we deref status

The, we call predicate; predicate first checks if status is null (when it can never be because of deref/NPE)

Then we do comparison.

I think it would be good idea to clarify in Javadoc (and code) either one or the other situation

Predicate<TaskStatusPlus> predicate "must be able to accept null", OR, predicate "will never be called with null".

thanks, added here

jasonk000 · 2023-08-09T18:21:23Z

 {
  public static final String KILL_TASK_TYPE = "kill";
  public static final String TASK_ID_PREFIX = "coordinator-issued";
+  public static final Predicate<TaskStatusPlus> TASK_PREDICATE =


Similar here, I think we should give the name a more clear name, IS_KILL_TASK?

If we choose to have a more specific name, I think it should say IS_AUTO_KILL_TASK instead of IS_KILL_TASK as the predicate excludes manual kill tasks

jasonk000 · 2023-08-09T18:25:13Z

+      final long currentTimeMillis = System.currentTimeMillis();
+      if (CollectionUtils.isNullOrEmpty(dataSourcesToKill)) {
+        log.debug("No eligible datasource to kill unused segments.");
+      } else if (lastKillTime + period > currentTimeMillis) {


Should this check be pulled out earlier in the function; ie: we can avoid calculation altogether if the kill time was too recent. Not sure if I missed something.

jasonk000 · 2023-08-09T18:31:11Z

+        availableKillTaskSlots < dataSourcesToKill.size()
+            ? StringUtils.format(
+            " Datasources skipped: %s",
+            ImmutableList.copyOf(dataSourcesToKill).subList(submittedTasks, dataSourcesToKill.size())


fwiw, I think this might need to be wrapped in an log.isDebug(), since the string format will likely be executed even if debug is not enabled.

jasonk000

swapping to request changes based on prior comments

suneet-s

Request changes for

Remove the use of the constant TaskStats.EMPTY across coordinator runs
Please add these new metrics to the metrics.md file, perhaps next to the compactTask/* metrics as these are related

suneet-s · 2023-08-09T22:28:10Z

 {
  public static final String KILL_TASK_TYPE = "kill";
  public static final String TASK_ID_PREFIX = "coordinator-issued";
+  public static final Predicate<TaskStatusPlus> TASK_PREDICATE =


If we choose to have a more specific name, I think it should say IS_AUTO_KILL_TASK instead of IS_KILL_TASK as the predicate excludes manual kill tasks

Co-authored-by: Suneet Saldanha <suneet@apache.org>

…CoordinatorDutyUtils.java Co-authored-by: Suneet Saldanha <suneet@apache.org>

suneet-s

LGTM after docs are added for the new metrics. Thanks @zachjsh !

zachjsh · 2023-08-10T18:10:58Z

LGTM after docs are added for the new metrics. Thanks @zachjsh !

thanks @suneet-s . Added the docs you requested. Wondering if the metric prefix should be changes from killTask to autoKillTask?

jasonk000

approved with a clarification/question

jasonk000 · 2023-08-10T18:29:21Z

+      }
+
+      if (lastKillTime + period > currentTimeMillis) {
+        log.debug("Skipping kill of unused segments as kill period has not elapsed yet.");


I'm not sure why we check this twice (above, at line 117-119). Is it because we want to capture stats?

fixed thanks!

suneet-s

LGTM with a suggestion on changing the metric names to match the auto-compaction metric naming scheme a little better

suneet-s · 2023-08-10T18:39:29Z

LGTM after docs are added for the new metrics. Thanks @zachjsh !

thanks @suneet-s . Added the docs you requested. Wondering if the metric prefix should be changes from killTask to autoKillTask?

I think killTask is fine as this matches the naming scheme of the auto-compaction metrics emitted by the CompactSegments duty

Co-authored-by: Suneet Saldanha <suneet@apache.org>

…stats

### Description Added the following metrics, which are calculated from the `KillUnusedSegments` coordinatorDuty `"killTask/availableSlot/count"`: calculates the number remaining task slots available for auto kill `"killTask/maxSlot/count"`: calculates the maximum number of tasks available for auto kill `"killTask/task/count"`: calculates the number of tasks submitted by auto kill. #### Release note NEW: metrics added for auto kill `"killTask/availableSlot/count"`: calculates the number remaining task slots available for auto kill `"killTask/maxSlot/count"`: calculates the maximum number of tasks available for auto kill `"killTask/task/count"`: calculates the number of tasks submitted by auto kill.

zachjsh added 2 commits August 8, 2023 16:55

* address easy review comments from previous patch

ac4a99e

Add stats to KillUnusedSegments coordinator duty

c21cfc7

zachjsh requested review from jasonk000, maytasm and suneet-s August 8, 2023 22:35

jasonk000 approved these changes Aug 9, 2023

View reviewed changes

jasonk000 reviewed Aug 9, 2023

View reviewed changes

jasonk000 requested changes Aug 9, 2023

View reviewed changes

suneet-s added Area - Documentation Area - Operations Area - Metrics/Event Emitting labels Aug 9, 2023

suneet-s requested changes Aug 9, 2023

View reviewed changes

* address review comments

2f4df30

github-actions Bot removed the Area - Documentation label Aug 9, 2023

zachjsh and others added 3 commits August 10, 2023 00:17

Apply suggestions from code review

17bc54e

Co-authored-by: Suneet Saldanha <suneet@apache.org>

Update server/src/main/java/org/apache/druid/server/coordinator/duty/…

1eabfdf

…CoordinatorDutyUtils.java Co-authored-by: Suneet Saldanha <suneet@apache.org>

* address review commments

eb72c34

zachjsh requested review from jasonk000 and suneet-s August 10, 2023 04:31

suneet-s reviewed Aug 10, 2023

View reviewed changes

* add documentation for new metrics

c3f3a00

github-actions Bot added the Area - Documentation label Aug 10, 2023

zachjsh requested a review from suneet-s August 10, 2023 18:12

* fix metrics.md formatting

199c3ae

jasonk000 approved these changes Aug 10, 2023

View reviewed changes

suneet-s approved these changes Aug 10, 2023

View reviewed changes

Comment thread docs/operations/metrics.md Outdated

Comment thread server/src/main/java/org/apache/druid/server/coordinator/stats/Stats.java Outdated

zachjsh and others added 2 commits August 10, 2023 15:28

Apply suggestions from code review

175c0ae

Co-authored-by: Suneet Saldanha <suneet@apache.org>

* remove duplicate check

127e262

Merge remote-tracking branch 'origin/auto-kill-stats' into auto-kill-…

5030f0d

…stats

zachjsh merged commit 82d82df into apache:master Aug 10, 2023

zachjsh deleted the auto-kill-stats branch August 10, 2023 22:36

LakshSingla added this to the 28.0 milestone Oct 12, 2023

LakshSingla mentioned this pull request Nov 4, 2023

[DRAFT] 28.0.0 release notes #15326

Closed


		private static final Logger LOG = new Logger(CompactSegments.class);

		private static final Predicate<TaskStatusPlus> TASK_PREDICATE =

Conversation

zachjsh commented Aug 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Release note

Uh oh!

jasonk000 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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonk000 left a comment

Choose a reason for hiding this comment

Uh oh!

suneet-s left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

suneet-s left a comment

Choose a reason for hiding this comment

Uh oh!

zachjsh commented Aug 10, 2023

Uh oh!

jasonk000 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!

suneet-s left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

suneet-s commented Aug 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zachjsh commented Aug 8, 2023 •

edited

Loading