Track the actor that triggers the minion task #14829

shounakmk219 · 2025-01-16T16:56:54Z

Description

Minion task can be triggered by 3 ways as of now

Cron schedule
Manual schedule
Adhoc task execution

To identify which actor triggered a particular task run, this PR injects the information in task configs so that it gets tracked in zk for reference.

The triggeredBy info is also made available on TaskDebugInfo and SubtaskDebugInfo.

Refactor on `PinotTaskManager`

There were too many derivations of schedule methods in PinotTaskManager based on what combination of tables and task types are supposed to be scheduled. This made the changes on schedule inputs hard to manage.
This PR adds a wrapper class TaskSchedulingContext that will be passed to only one scheduleTasks() method to achieve all the derivations.

codecov-commenter · 2025-01-16T19:04:31Z

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 56.21%. Comparing base (59551e4) to head (bf4d5ad).
Report is 1673 commits behind head on master.

Files with missing lines	Patch %	Lines
...va/org/apache/pinot/spi/utils/CommonConstants.java	0.00%	2 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (59551e4) and HEAD (bf4d5ad). Click for more details.

HEAD has 22 uploads less than BASE

Flag BASE (59551e4) HEAD (bf4d5ad)

integration 7 5

integration2 3 0

temurin 12 8

java-21 7 5

skip-bytebuffers-true 3 2

skip-bytebuffers-false 7 4

unittests 5 3

java-11 5 3

unittests2 3 0

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #14829      +/-   ##
============================================
- Coverage     61.75%   56.21%   -5.54%     
- Complexity      207      802     +595     
============================================
  Files          2436     2125     -311     
  Lines        133233   114582   -18651     
  Branches      20636    18444    -2192     
============================================
- Hits          82274    64410   -17864     
- Misses        44911    44917       +6     
+ Partials       6048     5255     -793

Flag	Coverage Δ
custom-integration1	`100.00% <ø> (+99.99%)`	⬆️
integration	`100.00% <ø> (+99.99%)`	⬆️
integration1	`100.00% <ø> (+99.99%)`	⬆️
integration2	`?`
java-11	`56.15% <0.00%> (-5.56%)`	⬇️
java-21	`56.07% <0.00%> (-5.55%)`	⬇️
skip-bytebuffers-false	`56.20% <0.00%> (-5.54%)`	⬇️
skip-bytebuffers-true	`56.02% <0.00%> (+28.29%)`	⬆️
temurin	`56.21% <0.00%> (-5.54%)`	⬇️
unittests	`56.21% <0.00%> (-5.54%)`	⬇️
unittests1	`56.21% <0.00%> (+9.31%)`	⬆️
unittests2	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

KKcorps · 2025-01-30T09:40:26Z

...controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java

-   * It might be called from the non-leader controller.
-   * Returns a map from the task type to the {@link TaskSchedulingInfo} of tasks scheduled.
-   */
-  public synchronized Map<String, TaskSchedulingInfo> scheduleAllTasksForAllTables(@Nullable String minionInstanceTag) {


I would suggest rather than removing them add a @deprecated annotation on top, along with the method that they should be using. We can remove this in next release

ok, will revert removed methods

KKcorps · 2025-01-30T09:40:57Z

...controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java

@@ -114,6 +114,10 @@ public class PinotTaskManager extends ControllerPeriodicTask<Void> {

  private final TaskManagerStatusCache<TaskGeneratorMostRecentRunInfo> _taskManagerStatusCache;

+  public enum Triggers {


Better to move this outside

KKcorps · 2025-01-30T09:41:22Z

...controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java

    Map<String, List<TableConfig>> enabledTableConfigMap = new HashMap<>();
-    for (String tableNameWithType : tableNamesWithType) {
+    Map<String, Set<String>> tableToTasksMap = context.getTableToTaskNamesMap();
+    if (context.getTableToTaskNamesMap().isEmpty()) {


can simply use tableToTasksMap here

KKcorps · 2025-01-30T09:41:39Z

...controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java

+    if (context.getTableToTaskNamesMap().isEmpty()) {
+      _pinotHelixResourceManager.getAllTables().forEach(table -> tableToTasksMap.put(table, null));
+    }
+    for (Map.Entry<String, Set<String>> entry : context.getTableToTaskNamesMap().entrySet()) {


same as before

KKcorps · 2025-01-30T09:47:59Z

...controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java

  /**
   * Helper method to schedule tasks (all task types) for the given tables that have the tasks enabled.
   * Returns a map from the task type to the {@link TaskSchedulingInfo} of the tasks scheduled.
   */
-  protected synchronized Map<String, TaskSchedulingInfo> scheduleTasks(List<String> tableNamesWithType,
-      boolean isLeader, @Nullable String minionInstanceTag) {
+  public synchronized Map<String, TaskSchedulingInfo> scheduleTasks(TaskSchedulingContext context) {


do we need this to be public? let's keep it as protected

This method is being called everywhere hence kept it public

KKcorps · 2025-01-30T09:52:36Z

...controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java

-    for (String tableNameWithType : tableNamesWithType) {
+    Map<String, Set<String>> tableToTasksMap = context.getTableToTaskNamesMap();
+    if (context.getTableToTaskNamesMap().isEmpty()) {
+      _pinotHelixResourceManager.getAllTables().forEach(table -> tableToTasksMap.put(table, null));


Adding an entry for all tables is probably not optimal, see if there is a way around this

good point, let me refactor the code to avoid adding entries

KKcorps · 2025-01-30T12:53:04Z

Add missing license headers

KKcorps · 2025-02-03T07:55:32Z

...controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java

+    Set<String> tasksToSchedule = context.getTasksToSchedule();
+    Set<String> finalTablesToSchedule = new HashSet<>();
+    if (tablesToSchedule != null) {
+      finalTablesToSchedule.addAll(tablesToSchedule);


nit: you can rename these as targetTables targetDatabases and consolidatedTables

instead of tablesToSchedule, databasesToSchedule and finalTablesToSchedule

KKcorps

LGTM with minor comments

shounakmk219 changed the title ~~Track the actor that triggers the task~~ Track the actor that triggers the minion task Jan 16, 2025

shounakmk219 added refactor minion labels Jan 16, 2025

shounakmk219 requested review from tibrewalpratik17, swaminathanmanish and KKcorps January 17, 2025 05:41

KKcorps reviewed Jan 30, 2025

View reviewed changes

shounakmk219 force-pushed the task-trigger-tracking branch from 79a8879 to 7040e3b Compare January 30, 2025 14:13

KKcorps reviewed Feb 3, 2025

View reviewed changes

KKcorps approved these changes Feb 3, 2025

View reviewed changes

shounakmk219 added 3 commits February 5, 2025 15:19

Track the actor that triggered the task

6e9e2f4

add java docs

d7956b8

simplified TaskSchedulingContext

bf4d5ad

shounakmk219 force-pushed the task-trigger-tracking branch from 1fc7a9d to bf4d5ad Compare February 5, 2025 09:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track the actor that triggers the minion task #14829

Track the actor that triggers the minion task #14829

shounakmk219 commented Jan 16, 2025

codecov-commenter commented Jan 16, 2025 •

edited

Loading

KKcorps Jan 30, 2025

shounakmk219 Jan 30, 2025

KKcorps Jan 30, 2025

KKcorps Jan 30, 2025

KKcorps Jan 30, 2025

KKcorps Jan 30, 2025

shounakmk219 Jan 30, 2025

KKcorps Jan 30, 2025

shounakmk219 Jan 30, 2025

KKcorps commented Jan 30, 2025

KKcorps Feb 3, 2025

KKcorps left a comment

		@@ -114,6 +114,10 @@ public class PinotTaskManager extends ControllerPeriodicTask<Void> {

		private final TaskManagerStatusCache<TaskGeneratorMostRecentRunInfo> _taskManagerStatusCache;

		public enum Triggers {

Track the actor that triggers the minion task #14829

Are you sure you want to change the base?

Track the actor that triggers the minion task #14829

Conversation

shounakmk219 commented Jan 16, 2025

Description

Refactor on PinotTaskManager

codecov-commenter commented Jan 16, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KKcorps commented Jan 30, 2025

Choose a reason for hiding this comment

KKcorps left a comment

Choose a reason for hiding this comment

Refactor on `PinotTaskManager`

codecov-commenter commented Jan 16, 2025 •

edited

Loading