You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "tarun11Mavani (via GitHub)" <gi...@apache.org> on 2024/10/14 16:23:59 UTC
[PR] #13941 Added validation for minionInstaceTag while creating a new TableTask [pinot]
tarun11Mavani opened a new pull request, #14228:
URL: https://github.com/apache/pinot/pull/14228
When creating a new Task with in TableTaskConfig, validate that a minion tenant with the same tag exists.
If a minion with same minionInstaceTag doesn't exist, we will fail the table creation.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Added validation for minionInstanceTag while updating / creating table-config [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #14228:
URL: https://github.com/apache/pinot/pull/14228#discussion_r1808445138
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManagerStatelessTest.java:
##########
@@ -662,6 +665,58 @@ public void testValidateTenantConfig() {
resetServerTags();
}
+ @Test
+ public void testValidateTableTaskMinionInstanceTagConfig() {
+ TableConfig realtimeTableConfig = new TableConfigBuilder(TableType.REALTIME).setTableName(RAW_TABLE_NAME).build();
+
+ Map<String, String> segmentGenerationAndPushTaskConfig =
+ Map.of("invalidRecordsThresholdCount", "1", "schedule", "0 */30 * ? * *", "tableMaxNumTasks", "1",
+ "validDocIdsType", "SNAPSHOT");
+
+ Map<String, String> upsertCompactionTask =
+ Map.of("invalidRecordsThresholdCount", "1", "schedule", "0 */30 * ? * *", "tableMaxNumTasks", "1",
+ "validDocIdsType", "SNAPSHOT", "minionInstanceTag", "minionTenant");
+
+ Map<String, String> segmentGenerationAndPushTaskConfig2 =
+ Map.of("schedule", "0 */30 * ? * *", "tableMaxNumTasks", "1", "validDocIdsType", "SNAPSHOT",
+ "minionInstanceTag", "anotherMinionTenant");
+
+ // Minion instance tag set but no minion present
+ realtimeTableConfig.setTaskConfig(new TableTaskConfig(
+ ImmutableMap.of(MinionConstants.RealtimeToOfflineSegmentsTask.TASK_TYPE, upsertCompactionTask)));
+ assertThrows(InvalidTableConfigException.class,
+ () -> _helixResourceManager.validateTableTaskMinionInstanceTagConfig(realtimeTableConfig));
+
+ // Valid minion instance tag with instances
+ addMinionInstance();
+ _helixResourceManager.validateTableTaskMinionInstanceTagConfig(realtimeTableConfig);
+
+ //Untag minion instance
+ untagMinions();
+ realtimeTableConfig.setTaskConfig(new TableTaskConfig(
+ ImmutableMap.of(MinionConstants.RealtimeToOfflineSegmentsTask.TASK_TYPE, segmentGenerationAndPushTaskConfig)));
+ _helixResourceManager.validateTableTaskMinionInstanceTagConfig(realtimeTableConfig);
+
+ realtimeTableConfig.setTaskConfig(new TableTaskConfig(
+ ImmutableMap.of(MinionConstants.RealtimeToOfflineSegmentsTask.TASK_TYPE, segmentGenerationAndPushTaskConfig2)));
+ assertThrows(InvalidTableConfigException.class,
Review Comment:
nit: let's also assert the exception message here
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Added validation for minionInstanceTag while updating / creating table-config [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #14228:
URL: https://github.com/apache/pinot/pull/14228#discussion_r1808498727
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManagerStatelessTest.java:
##########
@@ -662,6 +665,73 @@ public void testValidateTenantConfig() {
resetServerTags();
}
+ @Test
+ public void testValidateTableTaskMinionInstanceTagConfig() {
+ TableConfig realtimeTableConfig = new TableConfigBuilder(TableType.REALTIME).setTableName(RAW_TABLE_NAME).build();
+
+ Map<String, String> segmentGenerationAndPushTaskConfig =
+ Map.of("invalidRecordsThresholdCount", "1", "schedule", "0 */30 * ? * *", "tableMaxNumTasks", "1",
+ "validDocIdsType", "SNAPSHOT");
+
+ Map<String, String> upsertCompactionTask =
+ Map.of("invalidRecordsThresholdCount", "1", "schedule", "0 */30 * ? * *", "tableMaxNumTasks", "1",
+ "validDocIdsType", "SNAPSHOT", "minionInstanceTag", "minionTenant");
+
+ Map<String, String> segmentGenerationAndPushTaskConfig2 =
+ Map.of("schedule", "0 */30 * ? * *", "tableMaxNumTasks", "1", "validDocIdsType", "SNAPSHOT",
+ "minionInstanceTag", "anotherMinionTenant");
+
+ // Minion instance tag set but no minion present
+ realtimeTableConfig.setTaskConfig(new TableTaskConfig(
+ ImmutableMap.of(MinionConstants.RealtimeToOfflineSegmentsTask.TASK_TYPE, upsertCompactionTask)));
+
+ assertThrows(InvalidTableConfigException.class, () -> {
+ try {
+ _helixResourceManager.validateTableTaskMinionInstanceTagConfig(realtimeTableConfig);
+ } catch (InvalidTableConfigException e) {
+ assertEquals(e.getMessage(),
+ "Failed to find minion instances with tag: minionTenant for table: testTable_REALTIME");
+ throw e;
+ }
+ });
+
+ // Valid minion instance tag with instances
+ addMinionInstance();
+ _helixResourceManager.validateTableTaskMinionInstanceTagConfig(realtimeTableConfig);
+
+ //Untag minion instance
+ untagMinions();
+ realtimeTableConfig.setTaskConfig(new TableTaskConfig(
+ ImmutableMap.of(MinionConstants.RealtimeToOfflineSegmentsTask.TASK_TYPE, segmentGenerationAndPushTaskConfig)));
+ _helixResourceManager.validateTableTaskMinionInstanceTagConfig(realtimeTableConfig);
+
+ realtimeTableConfig.setTaskConfig(new TableTaskConfig(
+ ImmutableMap.of(MinionConstants.RealtimeToOfflineSegmentsTask.TASK_TYPE, segmentGenerationAndPushTaskConfig2)));
+ assertThrows(InvalidTableConfigException.class, () -> {
+ try {
+ _helixResourceManager.validateTableTaskMinionInstanceTagConfig(realtimeTableConfig);
+ } catch (InvalidTableConfigException e) {
+ assertEquals(e.getMessage(),
+ "Failed to find minion instances with tag: anotherMinionTenant for table: testTable_REALTIME");
+ throw e;
+ }
Review Comment:
We should catch and throw any other exception encountered.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Added validation for minionInstanceTag while updating / creating table-config [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #14228:
URL: https://github.com/apache/pinot/pull/14228#discussion_r1808510006
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManagerStatelessTest.java:
##########
@@ -662,6 +665,73 @@ public void testValidateTenantConfig() {
resetServerTags();
}
+ @Test
+ public void testValidateTableTaskMinionInstanceTagConfig() {
+ TableConfig realtimeTableConfig = new TableConfigBuilder(TableType.REALTIME).setTableName(RAW_TABLE_NAME).build();
+
+ Map<String, String> segmentGenerationAndPushTaskConfig =
+ Map.of("invalidRecordsThresholdCount", "1", "schedule", "0 */30 * ? * *", "tableMaxNumTasks", "1",
+ "validDocIdsType", "SNAPSHOT");
+
+ Map<String, String> upsertCompactionTask =
+ Map.of("invalidRecordsThresholdCount", "1", "schedule", "0 */30 * ? * *", "tableMaxNumTasks", "1",
+ "validDocIdsType", "SNAPSHOT", "minionInstanceTag", "minionTenant");
+
+ Map<String, String> segmentGenerationAndPushTaskConfig2 =
+ Map.of("schedule", "0 */30 * ? * *", "tableMaxNumTasks", "1", "validDocIdsType", "SNAPSHOT",
+ "minionInstanceTag", "anotherMinionTenant");
+
+ // Minion instance tag set but no minion present
+ realtimeTableConfig.setTaskConfig(new TableTaskConfig(
+ ImmutableMap.of(MinionConstants.RealtimeToOfflineSegmentsTask.TASK_TYPE, upsertCompactionTask)));
+
+ assertThrows(InvalidTableConfigException.class, () -> {
+ try {
+ _helixResourceManager.validateTableTaskMinionInstanceTagConfig(realtimeTableConfig);
+ } catch (InvalidTableConfigException e) {
+ assertEquals(e.getMessage(),
+ "Failed to find minion instances with tag: minionTenant for table: testTable_REALTIME");
+ throw e;
+ }
+ });
+
+ // Valid minion instance tag with instances
+ addMinionInstance();
+ _helixResourceManager.validateTableTaskMinionInstanceTagConfig(realtimeTableConfig);
+
+ //Untag minion instance
+ untagMinions();
+ realtimeTableConfig.setTaskConfig(new TableTaskConfig(
+ ImmutableMap.of(MinionConstants.RealtimeToOfflineSegmentsTask.TASK_TYPE, segmentGenerationAndPushTaskConfig)));
+ _helixResourceManager.validateTableTaskMinionInstanceTagConfig(realtimeTableConfig);
+
+ realtimeTableConfig.setTaskConfig(new TableTaskConfig(
+ ImmutableMap.of(MinionConstants.RealtimeToOfflineSegmentsTask.TASK_TYPE, segmentGenerationAndPushTaskConfig2)));
+ assertThrows(InvalidTableConfigException.class, () -> {
+ try {
+ _helixResourceManager.validateTableTaskMinionInstanceTagConfig(realtimeTableConfig);
+ } catch (InvalidTableConfigException e) {
+ assertEquals(e.getMessage(),
+ "Failed to find minion instances with tag: anotherMinionTenant for table: testTable_REALTIME");
+ throw e;
+ }
Review Comment:
Discussed offline, this will be handled.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] #13941 Added validation for minionInstaceTag while creating a new TableTask [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #14228:
URL: https://github.com/apache/pinot/pull/14228#issuecomment-2420743862
cc @swaminathanmanish
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Added validation for minionInstanceTag while updating / creating table-config [pinot]
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on code in PR #14228:
URL: https://github.com/apache/pinot/pull/14228#discussion_r1808728067
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -1815,6 +1821,28 @@ void validateTableTenantConfig(TableConfig tableConfig) {
}
}
+ @VisibleForTesting
+ void validateTableTaskMinionInstanceTagConfig(TableConfig tableConfig) {
+
+ List<InstanceConfig> allMinionWorkerInstanceConfigs = getAllMinionInstanceConfigs();
+
+ //extract all minionInstanceTags from allMinionWorkerInstanceConfigs
+ Set<String> minionInstanceTagSet = allMinionWorkerInstanceConfigs.stream().map(InstanceConfig::getTags)
+ .collect(HashSet::new, Set::addAll, Set::addAll);
+
+ if (tableConfig.getTaskConfig() != null && tableConfig.getTaskConfig().getTaskTypeConfigsMap() != null) {
+ tableConfig.getTaskConfig().getTaskTypeConfigsMap().forEach((taskType, taskTypeConfig) -> {
+ String taskInstanceTag = taskTypeConfig.getOrDefault(PinotTaskManager.MINION_INSTANCE_TAG_CONFIG,
+ CommonConstants.Helix.UNTAGGED_MINION_INSTANCE);
+ if (!minionInstanceTagSet.contains(taskInstanceTag)) {
+ throw new InvalidTableConfigException(
+ String.format("Failed to find minion instances with tag: %s for table: %s", taskInstanceTag,
Review Comment:
Thanks for the pointer !
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Added validation for minionInstanceTag while updating / creating table-config [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #14228:
URL: https://github.com/apache/pinot/pull/14228#discussion_r1808684382
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -1815,6 +1821,28 @@ void validateTableTenantConfig(TableConfig tableConfig) {
}
}
+ @VisibleForTesting
+ void validateTableTaskMinionInstanceTagConfig(TableConfig tableConfig) {
+
+ List<InstanceConfig> allMinionWorkerInstanceConfigs = getAllMinionInstanceConfigs();
+
+ //extract all minionInstanceTags from allMinionWorkerInstanceConfigs
+ Set<String> minionInstanceTagSet = allMinionWorkerInstanceConfigs.stream().map(InstanceConfig::getTags)
+ .collect(HashSet::new, Set::addAll, Set::addAll);
+
+ if (tableConfig.getTaskConfig() != null && tableConfig.getTaskConfig().getTaskTypeConfigsMap() != null) {
+ tableConfig.getTaskConfig().getTaskTypeConfigsMap().forEach((taskType, taskTypeConfig) -> {
+ String taskInstanceTag = taskTypeConfig.getOrDefault(PinotTaskManager.MINION_INSTANCE_TAG_CONFIG,
+ CommonConstants.Helix.UNTAGGED_MINION_INSTANCE);
+ if (!minionInstanceTagSet.contains(taskInstanceTag)) {
+ throw new InvalidTableConfigException(
+ String.format("Failed to find minion instances with tag: %s for table: %s", taskInstanceTag,
Review Comment:
We do check valid minionInstanceTag during task-generation which is common to adhoc trigger afair -- https://github.com/apache/pinot/blob/9db16254975c9cc46573c51fc16d8d34a2275567/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java#L662-L665
This patch is to proactively prevent table configs from having invalid minionInstanceTag values.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Added validation for minionInstanceTag while updating / creating table-config [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 merged PR #14228:
URL: https://github.com/apache/pinot/pull/14228
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] #13941 Added validation for minionInstaceTag while creating a new TableTask [pinot]
Posted by "tarun11Mavani (via GitHub)" <gi...@apache.org>.
tarun11Mavani commented on PR #14228:
URL: https://github.com/apache/pinot/pull/14228#issuecomment-2413481936
cc @tibrewalpratik17
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] #13941 Added validation for minionInstaceTag while creating a new TableTask [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #14228:
URL: https://github.com/apache/pinot/pull/14228#issuecomment-2411804442
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/14228?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 55.34%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`f9e611f`)](https://app.codecov.io/gh/apache/pinot/commit/f9e611fb0978aa6d7bc6b386fde47a60ff87ea1b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1177 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (59551e4) and HEAD (f9e611f). Click for more details.
>
> <details><summary>HEAD has 53 uploads less than BASE</summary>
>
>| Flag | BASE (59551e4) | HEAD (f9e611f) |
>|------|------|------|
>|integration|7|0|
>|integration2|3|0|
>|temurin|12|1|
>|java-21|7|1|
>|skip-bytebuffers-true|3|1|
>|skip-bytebuffers-false|7|0|
>|unittests|5|1|
>|unittests1|2|1|
>|java-11|5|0|
>|unittests2|3|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #14228 +/- ##
============================================
- Coverage 61.75% 55.34% -6.41%
- Complexity 207 696 +489
============================================
Files 2436 2068 -368
Lines 133233 109222 -24011
Branches 20636 17293 -3343
============================================
- Hits 82274 60454 -21820
+ Misses 44911 43934 -977
+ Partials 6048 4834 -1214
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/14228/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/14228/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/14228/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/14228/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/14228/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/14228/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/14228/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.34% <ø> (-6.28%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/14228/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/14228/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.34% <ø> (+27.62%)` | :arrow_up: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/14228/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.34% <ø> (-6.41%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/14228/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.34% <ø> (-6.40%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/14228/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.34% <ø> (+8.45%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/14228/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/14228?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Added validation for minionInstanceTag while updating / creating table-config [pinot]
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on code in PR #14228:
URL: https://github.com/apache/pinot/pull/14228#discussion_r1808636526
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -1815,6 +1821,28 @@ void validateTableTenantConfig(TableConfig tableConfig) {
}
}
+ @VisibleForTesting
+ void validateTableTaskMinionInstanceTagConfig(TableConfig tableConfig) {
+
+ List<InstanceConfig> allMinionWorkerInstanceConfigs = getAllMinionInstanceConfigs();
+
+ //extract all minionInstanceTags from allMinionWorkerInstanceConfigs
+ Set<String> minionInstanceTagSet = allMinionWorkerInstanceConfigs.stream().map(InstanceConfig::getTags)
+ .collect(HashSet::new, Set::addAll, Set::addAll);
+
+ if (tableConfig.getTaskConfig() != null && tableConfig.getTaskConfig().getTaskTypeConfigsMap() != null) {
+ tableConfig.getTaskConfig().getTaskTypeConfigsMap().forEach((taskType, taskTypeConfig) -> {
+ String taskInstanceTag = taskTypeConfig.getOrDefault(PinotTaskManager.MINION_INSTANCE_TAG_CONFIG,
+ CommonConstants.Helix.UNTAGGED_MINION_INSTANCE);
Review Comment:
Assuming existing integ tests will handle default case when there are no tags .
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -1815,6 +1821,28 @@ void validateTableTenantConfig(TableConfig tableConfig) {
}
}
+ @VisibleForTesting
+ void validateTableTaskMinionInstanceTagConfig(TableConfig tableConfig) {
+
+ List<InstanceConfig> allMinionWorkerInstanceConfigs = getAllMinionInstanceConfigs();
+
+ //extract all minionInstanceTags from allMinionWorkerInstanceConfigs
+ Set<String> minionInstanceTagSet = allMinionWorkerInstanceConfigs.stream().map(InstanceConfig::getTags)
+ .collect(HashSet::new, Set::addAll, Set::addAll);
+
+ if (tableConfig.getTaskConfig() != null && tableConfig.getTaskConfig().getTaskTypeConfigsMap() != null) {
+ tableConfig.getTaskConfig().getTaskTypeConfigsMap().forEach((taskType, taskTypeConfig) -> {
+ String taskInstanceTag = taskTypeConfig.getOrDefault(PinotTaskManager.MINION_INSTANCE_TAG_CONFIG,
+ CommonConstants.Helix.UNTAGGED_MINION_INSTANCE);
+ if (!minionInstanceTagSet.contains(taskInstanceTag)) {
+ throw new InvalidTableConfigException(
+ String.format("Failed to find minion instances with tag: %s for table: %s", taskInstanceTag,
Review Comment:
How will this error surface for adhoc triggers ? (Execute call) where task params are in the payload .
cc @tibrewalpratik17
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Added validation for minionInstanceTag while updating / creating table-config [pinot]
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on PR #14228:
URL: https://github.com/apache/pinot/pull/14228#issuecomment-2426421677
> cc @swaminathanmanish
Taking a look .
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Added validation for minionInstanceTag while updating / creating table-config [pinot]
Posted by "tarun11Mavani (via GitHub)" <gi...@apache.org>.
tarun11Mavani commented on code in PR #14228:
URL: https://github.com/apache/pinot/pull/14228#discussion_r1808730345
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -1815,6 +1821,28 @@ void validateTableTenantConfig(TableConfig tableConfig) {
}
}
+ @VisibleForTesting
+ void validateTableTaskMinionInstanceTagConfig(TableConfig tableConfig) {
+
+ List<InstanceConfig> allMinionWorkerInstanceConfigs = getAllMinionInstanceConfigs();
+
+ //extract all minionInstanceTags from allMinionWorkerInstanceConfigs
+ Set<String> minionInstanceTagSet = allMinionWorkerInstanceConfigs.stream().map(InstanceConfig::getTags)
+ .collect(HashSet::new, Set::addAll, Set::addAll);
+
+ if (tableConfig.getTaskConfig() != null && tableConfig.getTaskConfig().getTaskTypeConfigsMap() != null) {
+ tableConfig.getTaskConfig().getTaskTypeConfigsMap().forEach((taskType, taskTypeConfig) -> {
+ String taskInstanceTag = taskTypeConfig.getOrDefault(PinotTaskManager.MINION_INSTANCE_TAG_CONFIG,
+ CommonConstants.Helix.UNTAGGED_MINION_INSTANCE);
Review Comment:
Yes. They are being handled.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org