Add minionInstanceTag config to minion-tasks for resource isolation #12459

tibrewalpratik17 · 2024-02-21T10:43:38Z

Iabel:

feature
release-notes (**)

Currently, all minion-scheduled jobs run on minion_untagged node instances. This doesn't allow isolation among tables / tasks at minion level.
Adhoc-minion-tasks already acknowledge a task-config minionInstanceTag and schedule those adhoc-runs only on those set of instances. Extending the same for periodic-scheduled tasks.

Resolving some TODOs / bugs in this PR itself where if generateTasks schedules tasks for some tables we still end up emitting a failed metric for all the tables. This PR makes the change of iterating through all tables and emitting metrics for each one of them.

Now, instead of generating a single parent task at taskType level, the change generates a single parent task at taskType, minionInstanceTag level.

Example config:

"task": {
      "taskTypeConfigsMap": {
        "UpsertCompactionTask": {
          "invalidRecordsThresholdCount": "10000",
          "bufferTimePeriod": "0d",
          "schedule": "0 * * * * ?",
          "tableMaxNumTasks": "15",
          "minionInstanceTag": "dummy_MINION",
        }
      }
    },

Here "minionInstanceTag": "dummy_MINION", is pertaining to the change in this patch.

codecov-commenter · 2024-02-21T11:19:44Z

Codecov Report

Attention: Patch coverage is 4.91803% with 58 lines in your changes are missing coverage. Please review.

Project coverage is 61.57%. Comparing base (59551e4) to head (3479383).
Report is 190 commits behind head on master.

Files	Patch %	Lines
...controller/helix/core/minion/PinotTaskManager.java	0.00%	46 Missing ⚠️
...roller/api/resources/PinotTaskRestletResource.java	0.00%	5 Missing ⚠️
...helix/core/minion/generator/BaseTaskGenerator.java	0.00%	3 Missing ⚠️
...elix/core/minion/generator/TaskGeneratorUtils.java	50.00%	1 Missing and 2 partials ⚠️
...elix/core/minion/generator/PinotTaskGenerator.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #12459      +/-   ##
============================================
- Coverage     61.75%   61.57%   -0.19%     
+ Complexity      207      198       -9     
============================================
  Files          2436     2463      +27     
  Lines        133233   134773    +1540     
  Branches      20636    20827     +191     
============================================
+ Hits          82274    82982     +708     
- Misses        44911    45588     +677     
- Partials       6048     6203     +155

Flag	Coverage Δ
custom-integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration	`<0.01% <0.00%> (-0.01%)`	⬇️
integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration2	`0.00% <0.00%> (ø)`
java-11	`61.54% <4.91%> (-0.17%)`	⬇️
java-21	`61.45% <4.91%> (-0.18%)`	⬇️
skip-bytebuffers-false	`61.55% <4.91%> (-0.19%)`	⬇️
skip-bytebuffers-true	`61.43% <4.91%> (+33.70%)`	⬆️
temurin	`61.57% <4.91%> (-0.19%)`	⬇️
unittests	`61.56% <4.91%> (-0.19%)`	⬇️
unittests1	`46.16% <ø> (-0.73%)`	⬇️
unittests2	`27.95% <4.91%> (+0.22%)`	⬆️

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.

Jackie-Jiang

@snleee @swaminathanmanish Please also take a look

...roller/src/main/java/org/apache/pinot/controller/api/resources/PinotTaskRestletResource.java

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

Co-authored-by: Xiaotian (Jackie) Jiang <[email protected]>

tibrewalpratik17 · 2024-03-08T10:31:00Z

@snleee can you please review this too?

ankitsultana · 2024-03-18T20:48:15Z

@snleee @Jackie-Jiang : we'll be working on getting this merged this week, so would be great if you folks can share your feedback.

@tibrewalpratik17 : can you also share what's the test plan for this PR in the description?

swaminathanmanish

Thanks for adding this capability. Looks good overall !

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

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

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

swaminathanmanish · 2024-03-18T23:33:55Z

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

+    if (tableTaskConfig != null) {
+      Map<String, String> configs = tableTaskConfig.getConfigsForTaskType(getTaskType());
+      if (!configs.isEmpty()) {
+        return configs.getOrDefault(PinotTaskManager.MINION_INSTANCE_TAG_CONFIG,


Since the user has to manually fill this in, can we add table config validation to ensure that the minion tag configured here has actually been created?

If there are typos, it'll be some work to troubleshoot this, if not validated during config time.

It's difficult to add in TableConfigUtils as we don't have HelixManager present in that class.

I see. It would be ideal to enforce some validation especially given that user has to enter this for every taskConfig. Im curious if we have any validation for table tenant.
Can we keep track of this work?

Actually we do in PinotHelixResourceManager class -

pinot/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java

Lines 1781 to 1785 in af0e5b8

for (String tag : tagsToCheck) {

if (getInstancesWithTag(tag).isEmpty()) {

throw new InvalidTableConfigException(

"Failed to find instances with tag: " + tag + " for table: " + tableNameWithType);

}

But I think we should refactor to have it in the TableConfigUtils class. Tracking it here: #12698

...rc/test/java/org/apache/pinot/integration/tests/MergeRollupMinionClusterIntegrationTest.java

swaminathanmanish · 2024-03-18T23:54:10Z

minionInstanceTag

To get more clarity -
@Jackie-Jiang , @snleee, @tibrewalpratik17 - Why should minionInstanceTag be at taskTypeConfigsMap instead of being a table level config ? Do we want isolation at the table level or task level?

snleee · 2024-03-19T00:07:26Z

@swaminathanmanish +1 on your point. I think that this feature naturally fits better to the tenant config?

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

tibrewalpratik17 · 2024-03-19T22:41:21Z

To get more clarity -
@Jackie-Jiang , @snleee, @tibrewalpratik17 - Why should minionInstanceTag be at taskTypeConfigsMap instead of being a table level config ? Do we want isolation at the table level or task level?
@swaminathanmanish +1 on your point. I think that this feature naturally fits better to the tenant config?

Yes agreed to have a minionTenant concept as a table level config. Based on a discussion here - #11240 (comment) we would also like to give the flexibility to override it as task-level too just in case there are lot of tasks for a table and we want to isolate those too or group similar tasks of different tables in dedicated minion-nodes (may be different SKUs based on tasks).
I raised this patch to quickly give the option of isolating tasks first. Post this, can work on a patch to introduce minionTenant concept in Pinot. Anyways, since this task-level config will have higher precedence over table's tenant config, this change will be backward compatible even then. What do you all think about this?

Also, will address comments shortly and update the patch.

tibrewalpratik17 · 2024-03-21T22:08:06Z

hey @snleee @swaminathanmanish addressed comments. Please review!

swaminathanmanish · 2024-03-22T01:00:39Z

To get more clarity -
@Jackie-Jiang , @snleee, @tibrewalpratik17 - Why should minionInstanceTag be at taskTypeConfigsMap instead of being a table level config ? Do we want isolation at the table level or task level?
@swaminathanmanish +1 on your point. I think that this feature naturally fits better to the tenant config?

Yes agreed to have a minionTenant concept as a table level config. Based on a discussion here - #11240 (comment) we would also like to give the flexibility to override it as task-level too just in case there are lot of tasks for a table and we want to isolate those too or group similar tasks of different tables in dedicated minion-nodes (may be different SKUs based on tasks). I raised this patch to quickly give the option of isolating tasks first. Post this, can work on a patch to introduce minionTenant concept in Pinot. Anyways, since this task-level config will have higher precedence over table's tenant config, this change will be backward compatible even then. What do you all think about this?

Also, will address comments shortly and update the patch.

Thanks a lot @tibrewalpratik17 . Would you be able to create a github issue to track this effort?

swaminathanmanish

LGTM. Could you please create github issues to track the additional work?

tibrewalpratik17 · 2024-03-22T09:30:01Z

Thanks a lot @tibrewalpratik17 . Would you be able to create a github issue to track this effort?
LGTM. Could you please create github issues to track the additional work?

Tracking all tasks in #12698

snleee

LGTM. Can you fix the conflicts? once the test all passes, i can merge this.

vvivekiyer · 2024-03-27T04:22:23Z

...roller/src/main/java/org/apache/pinot/controller/api/resources/PinotTaskRestletResource.java

@@ -615,12 +615,15 @@ public Map<String, String> scheduleTasks(@ApiParam(value = "Task type") @QueryPa
    tableName = DatabaseUtils.translateTableName(tableName, headers);


Would it also make sense to extend this API to take in a minionInstanceTag?
I understand that tasks/execute API already provides that capability.

We plan to use this feature as follows:

Say all minion periodic tasks are usually executed in the minion_untagged hosts.

If our minion purge tasks are running behind for a table, we plan to deploy new set of hosts with a special tag e.g. minion_emergency, and use the schedule/execute API for one-off scheduling of these jobs on these hosts.

Hmm this is a good suggestion. Let me do it in a follow-up as this patch has been open from quite some time. Tracking improvements in #12698. Will add this one there.

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

tibrewalpratik17 · 2024-04-03T09:22:45Z

hey @snleee @vvivekiyer can one of you help me merge this?

vvivekiyer · 2024-04-03T16:40:22Z

Unrelated test failure in PulsarConsumerTest. Merging this PR.

npawar · 2024-05-22T20:11:06Z

Has documentation been added @tibrewalpratik17 ?

Add minionInstanceTag config to minions scheduled tasks

6e80d6e

Jackie-Jiang added feature release-notes Referenced by PRs that need attention when compiling the next release notes labels Feb 23, 2024

Jackie-Jiang requested a review from snleee February 23, 2024 00:50

tibrewalpratik17 added 2 commits February 29, 2024 01:32

fix npe

8d9b22d

refactor integration-tests

992145b

tibrewalpratik17 force-pushed the minion_instance_tag_feature branch from bbd9a7d to 992145b Compare February 29, 2024 10:37

tibrewalpratik17 changed the title ~~[WIP] Add minionInstanceTag config to minion-tasks for resource isolation~~ Add minionInstanceTag config to minion-tasks for resource isolation Feb 29, 2024

tibrewalpratik17 marked this pull request as ready for review February 29, 2024 20:09

Jackie-Jiang reviewed Mar 1, 2024

View reviewed changes

tibrewalpratik17 and others added 3 commits March 8, 2024 15:01

commit suggestion

7705f26

Co-authored-by: Xiaotian (Jackie) Jiang <[email protected]>

update API response type

5c7e650

address comments 3

34fe553

Merge branch 'master' into minion_instance_tag_feature

82d79c8

swaminathanmanish reviewed Mar 18, 2024

View reviewed changes

snleee reviewed Mar 19, 2024

View reviewed changes

address comments

09f1bcb

tibrewalpratik17 requested review from snleee and swaminathanmanish March 21, 2024 22:08

swaminathanmanish approved these changes Mar 22, 2024

View reviewed changes

tibrewalpratik17 mentioned this pull request Mar 22, 2024

MinionTenant as a table config #12698

Open

4 tasks

snleee approved these changes Mar 27, 2024

View reviewed changes

vvivekiyer reviewed Mar 27, 2024

View reviewed changes

zhtaoxiang reviewed Mar 27, 2024

View reviewed changes

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

tibrewalpratik17 added 2 commits April 2, 2024 00:38

Merge branch 'master' into minion_instance_tag_feature

8c15f3e

fix lint

3479383

vvivekiyer approved these changes Apr 2, 2024

View reviewed changes

Merge branch 'master' into minion_instance_tag_feature

f5762d9

tibrewalpratik17 force-pushed the minion_instance_tag_feature branch from 5732a04 to f5762d9 Compare April 3, 2024 14:30

vvivekiyer merged commit b825bb4 into apache:master Apr 3, 2024
18 of 19 checks passed

shounakmk219 mentioned this pull request Apr 4, 2024

Bugfix. Correct wrong method call from scheduleTask() to scheduleTaskForDatabase() #12791

Merged

tibrewalpratik17 deleted the minion_instance_tag_feature branch June 14, 2024 21:24

tibrewalpratik17 mentioned this pull request Jun 14, 2024

Update pinot-docs related to recent changes pinot-contrib/pinot-docs#351

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add minionInstanceTag config to minion-tasks for resource isolation #12459

Add minionInstanceTag config to minion-tasks for resource isolation #12459

tibrewalpratik17 commented Feb 21, 2024 •

edited

Loading

codecov-commenter commented Feb 21, 2024 •

edited

Loading

Jackie-Jiang left a comment

tibrewalpratik17 commented Mar 8, 2024

ankitsultana commented Mar 18, 2024

swaminathanmanish left a comment

swaminathanmanish Mar 18, 2024

tibrewalpratik17 Mar 21, 2024

swaminathanmanish Mar 22, 2024

tibrewalpratik17 Mar 22, 2024

swaminathanmanish commented Mar 18, 2024 •

edited

Loading

snleee commented Mar 19, 2024

tibrewalpratik17 commented Mar 19, 2024

tibrewalpratik17 commented Mar 21, 2024

swaminathanmanish commented Mar 22, 2024

swaminathanmanish left a comment

tibrewalpratik17 commented Mar 22, 2024

snleee left a comment •

edited

Loading

vvivekiyer Mar 27, 2024 •

edited

Loading

tibrewalpratik17 Apr 1, 2024

tibrewalpratik17 commented Apr 3, 2024

vvivekiyer commented Apr 3, 2024

npawar commented May 22, 2024

	for (String tag : tagsToCheck) {
	if (getInstancesWithTag(tag).isEmpty()) {
	throw new InvalidTableConfigException(
	"Failed to find instances with tag: " + tag + " for table: " + tableNameWithType);
	}

		@@ -615,12 +615,15 @@ public Map<String, String> scheduleTasks(@ApiParam(value = "Task type") @QueryPa
		tableName = DatabaseUtils.translateTableName(tableName, headers);

Add minionInstanceTag config to minion-tasks for resource isolation #12459

Add minionInstanceTag config to minion-tasks for resource isolation #12459

Conversation

tibrewalpratik17 commented Feb 21, 2024 • edited Loading

codecov-commenter commented Feb 21, 2024 • edited Loading

Codecov Report

Jackie-Jiang left a comment

Choose a reason for hiding this comment

tibrewalpratik17 commented Mar 8, 2024

ankitsultana commented Mar 18, 2024

swaminathanmanish left a comment

Choose a reason for hiding this comment

swaminathanmanish Mar 18, 2024

Choose a reason for hiding this comment

tibrewalpratik17 Mar 21, 2024

Choose a reason for hiding this comment

swaminathanmanish Mar 22, 2024

Choose a reason for hiding this comment

tibrewalpratik17 Mar 22, 2024

Choose a reason for hiding this comment

swaminathanmanish commented Mar 18, 2024 • edited Loading

snleee commented Mar 19, 2024

tibrewalpratik17 commented Mar 19, 2024

tibrewalpratik17 commented Mar 21, 2024

swaminathanmanish commented Mar 22, 2024

swaminathanmanish left a comment

Choose a reason for hiding this comment

tibrewalpratik17 commented Mar 22, 2024

snleee left a comment • edited Loading

Choose a reason for hiding this comment

vvivekiyer Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

tibrewalpratik17 Apr 1, 2024

Choose a reason for hiding this comment

tibrewalpratik17 commented Apr 3, 2024

vvivekiyer commented Apr 3, 2024

npawar commented May 22, 2024

tibrewalpratik17 commented Feb 21, 2024 •

edited

Loading

codecov-commenter commented Feb 21, 2024 •

edited

Loading

swaminathanmanish commented Mar 18, 2024 •

edited

Loading

snleee left a comment •

edited

Loading

vvivekiyer Mar 27, 2024 •

edited

Loading