You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/12/15 03:12:34 UTC
[GitHub] [incubator-pinot] Jackie-Jiang opened a new pull request #6352: Enhance task schedule api for single type/table support
Jackie-Jiang opened a new pull request #6352:
URL: https://github.com/apache/incubator-pinot/pull/6352
## Description
Added 2 optional query parameters to the task schedule API to schedule tasks for the given task type/table.
Examples:
```
POST /tasks/schedule?taskType=MyTask
POST /tasks/schedule?tableName=myTable_OFFLINE
POST /tasks/schedule?taskType=MyTask&tableName=myTable_OFFLINE
```
----------------------------------------------------------------
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.
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
[GitHub] [incubator-pinot] fx19880617 commented on a change in pull request #6352: Enhance task schedule api for single type/table support
Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #6352:
URL: https://github.com/apache/incubator-pinot/pull/6352#discussion_r544642472
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java
##########
@@ -63,99 +65,147 @@ public PinotTaskManager(PinotHelixTaskResourceManager helixTaskResourceManager,
}
/**
- * Returns the cluster info provider.
- * <p>
- * Cluster info provider might be useful when initializing task generators.
- *
- * @return Cluster info provider
+ * Returns the cluster info accessor.
+ * <p>Cluster info accessor can be used to initialize the task generator.
*/
public ClusterInfoAccessor getClusterInfoAccessor() {
return _clusterInfoAccessor;
}
/**
* Registers a task generator.
- * <p>
- * This method can be used to plug in custom task generators.
- *
- * @param pinotTaskGenerator Task generator to be registered
+ * <p>This method can be used to plug in custom task generators.
*/
- public void registerTaskGenerator(PinotTaskGenerator pinotTaskGenerator) {
- _taskGeneratorRegistry.registerTaskGenerator(pinotTaskGenerator);
+ public void registerTaskGenerator(PinotTaskGenerator taskGenerator) {
+ _taskGeneratorRegistry.registerTaskGenerator(taskGenerator);
}
/**
- * Public API to schedule tasks. It doesn't matter whether current pinot controller is leader.
+ * Public API to schedule tasks (all task types) for all tables. It might be called from the non-leader controller.
+ * Returns a map from the task type to the task scheduled.
*/
public synchronized Map<String, String> scheduleTasks() {
- Map<String, String> tasksScheduled = scheduleTasks(_pinotHelixResourceManager.getAllTables());
-
- // Reset the task because this method will be called from the Rest API instead of the periodic task scheduler
- // TODO: Clean up only the non-leader tables instead of all tables
- cleanUpTask();
- setUpTask();
-
- return tasksScheduled;
+ return scheduleTasks(_pinotHelixResourceManager.getAllTables(), false);
Review comment:
ic, so we actually always assume this call is from a non-leader.
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/SimpleMinionClusterIntegrationTest.java
##########
@@ -48,10 +48,7 @@
import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;
-import static org.testng.Assert.assertEquals;
-import static org.testng.Assert.assertFalse;
-import static org.testng.Assert.assertNotNull;
-import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.*;
Review comment:
ah, good to know :)
----------------------------------------------------------------
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.
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
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6352: Enhance task schedule api for single type/table support
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6352:
URL: https://github.com/apache/incubator-pinot/pull/6352#discussion_r544567942
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/SimpleMinionClusterIntegrationTest.java
##########
@@ -48,10 +48,7 @@
import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;
-import static org.testng.Assert.assertEquals;
-import static org.testng.Assert.assertFalse;
-import static org.testng.Assert.assertNotNull;
-import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.*;
Review comment:
Per the code style, if there are >= 5 imports, they will be combined into `*`
----------------------------------------------------------------
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.
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
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6352: Enhance task schedule api for single type/table support
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6352:
URL: https://github.com/apache/incubator-pinot/pull/6352#discussion_r544726131
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java
##########
@@ -63,99 +65,147 @@ public PinotTaskManager(PinotHelixTaskResourceManager helixTaskResourceManager,
}
/**
- * Returns the cluster info provider.
- * <p>
- * Cluster info provider might be useful when initializing task generators.
- *
- * @return Cluster info provider
+ * Returns the cluster info accessor.
+ * <p>Cluster info accessor can be used to initialize the task generator.
*/
public ClusterInfoAccessor getClusterInfoAccessor() {
return _clusterInfoAccessor;
}
/**
* Registers a task generator.
- * <p>
- * This method can be used to plug in custom task generators.
- *
- * @param pinotTaskGenerator Task generator to be registered
+ * <p>This method can be used to plug in custom task generators.
*/
- public void registerTaskGenerator(PinotTaskGenerator pinotTaskGenerator) {
- _taskGeneratorRegistry.registerTaskGenerator(pinotTaskGenerator);
+ public void registerTaskGenerator(PinotTaskGenerator taskGenerator) {
+ _taskGeneratorRegistry.registerTaskGenerator(taskGenerator);
}
/**
- * Public API to schedule tasks. It doesn't matter whether current pinot controller is leader.
+ * Public API to schedule tasks (all task types) for all tables. It might be called from the non-leader controller.
+ * Returns a map from the task type to the task scheduled.
*/
public synchronized Map<String, String> scheduleTasks() {
- Map<String, String> tasksScheduled = scheduleTasks(_pinotHelixResourceManager.getAllTables());
-
- // Reset the task because this method will be called from the Rest API instead of the periodic task scheduler
- // TODO: Clean up only the non-leader tables instead of all tables
- cleanUpTask();
- setUpTask();
-
- return tasksScheduled;
+ return scheduleTasks(_pinotHelixResourceManager.getAllTables(), false);
Review comment:
Yes, if it is not always the leader, we need to assume it is not the leader
----------------------------------------------------------------
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.
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
[GitHub] [incubator-pinot] codecov-io commented on pull request #6352: Enhance task schedule api for single type/table support
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #6352:
URL: https://github.com/apache/incubator-pinot/pull/6352#issuecomment-745040040
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6352?src=pr&el=h1) Report
> Merging [#6352](https://codecov.io/gh/apache/incubator-pinot/pull/6352?src=pr&el=desc) (1a3f264) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `20.89%`.
> The diff coverage is `51.88%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6352?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6352 +/- ##
===========================================
- Coverage 66.44% 45.55% -20.90%
===========================================
Files 1075 1291 +216
Lines 54773 62186 +7413
Branches 8168 9027 +859
===========================================
- Hits 36396 28331 -8065
- Misses 15700 31523 +15823
+ Partials 2677 2332 -345
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `45.55% <51.88%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6352?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6352/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6352/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
| [...org/apache/pinot/broker/queryquota/HitCounter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6352/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9IaXRDb3VudGVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...che/pinot/broker/queryquota/MaxHitRateTracker.java](https://codecov.io/gh/apache/incubator-pinot/pull/6352/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9NYXhIaXRSYXRlVHJhY2tlci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...ache/pinot/broker/queryquota/QueryQuotaEntity.java](https://codecov.io/gh/apache/incubator-pinot/pull/6352/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9RdWVyeVF1b3RhRW50aXR5LmphdmE=) | `0.00% <0.00%> (-50.00%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6352/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ceselector/StrictReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6352/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6352/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `22.22% <0.00%> (-26.62%)` | :arrow_down: |
| [...not/common/assignment/InstancePartitionsUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6352/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnNVdGlscy5qYXZh) | `64.28% <ø> (-8.89%)` | :arrow_down: |
| [...common/config/tuner/NoOpTableTableConfigTuner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6352/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL05vT3BUYWJsZVRhYmxlQ29uZmlnVHVuZXIuamF2YQ==) | `0.00% <ø> (ø)` | |
| ... and [1288 more](https://codecov.io/gh/apache/incubator-pinot/pull/6352/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6352?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6352?src=pr&el=footer). Last update [4183ffe...1a3f264](https://codecov.io/gh/apache/incubator-pinot/pull/6352?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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
[GitHub] [incubator-pinot] Jackie-Jiang merged pull request #6352: Enhance task schedule api for single type/table support
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged pull request #6352:
URL: https://github.com/apache/incubator-pinot/pull/6352
----------------------------------------------------------------
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.
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
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6352: Enhance task schedule api for single type/table support
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6352:
URL: https://github.com/apache/incubator-pinot/pull/6352#discussion_r544608302
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java
##########
@@ -63,99 +65,147 @@ public PinotTaskManager(PinotHelixTaskResourceManager helixTaskResourceManager,
}
/**
- * Returns the cluster info provider.
- * <p>
- * Cluster info provider might be useful when initializing task generators.
- *
- * @return Cluster info provider
+ * Returns the cluster info accessor.
+ * <p>Cluster info accessor can be used to initialize the task generator.
*/
public ClusterInfoAccessor getClusterInfoAccessor() {
return _clusterInfoAccessor;
}
/**
* Registers a task generator.
- * <p>
- * This method can be used to plug in custom task generators.
- *
- * @param pinotTaskGenerator Task generator to be registered
+ * <p>This method can be used to plug in custom task generators.
*/
- public void registerTaskGenerator(PinotTaskGenerator pinotTaskGenerator) {
- _taskGeneratorRegistry.registerTaskGenerator(pinotTaskGenerator);
+ public void registerTaskGenerator(PinotTaskGenerator taskGenerator) {
+ _taskGeneratorRegistry.registerTaskGenerator(taskGenerator);
}
/**
- * Public API to schedule tasks. It doesn't matter whether current pinot controller is leader.
+ * Public API to schedule tasks (all task types) for all tables. It might be called from the non-leader controller.
+ * Returns a map from the task type to the task scheduled.
*/
public synchronized Map<String, String> scheduleTasks() {
- Map<String, String> tasksScheduled = scheduleTasks(_pinotHelixResourceManager.getAllTables());
-
- // Reset the task because this method will be called from the Rest API instead of the periodic task scheduler
- // TODO: Clean up only the non-leader tables instead of all tables
- cleanUpTask();
- setUpTask();
-
- return tasksScheduled;
+ return scheduleTasks(_pinotHelixResourceManager.getAllTables(), false);
}
/**
- * Check the Pinot cluster status and schedule new tasks for the given tables.
- *
- * @param tableNamesWithType List of table names with type suffix
- * @return Map from task type to task scheduled
+ * 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 task scheduled.
*/
- private synchronized Map<String, String> scheduleTasks(List<String> tableNamesWithType) {
+ private synchronized Map<String, String> scheduleTasks(List<String> tableNamesWithType, boolean isLeader) {
_controllerMetrics.addMeteredGlobalValue(ControllerMeter.NUMBER_TIMES_SCHEDULE_TASKS_CALLED, 1L);
Set<String> taskTypes = _taskGeneratorRegistry.getAllTaskTypes();
- int numTaskTypes = taskTypes.size();
- Map<String, List<TableConfig>> enabledTableConfigMap = new HashMap<>(numTaskTypes);
-
- for (String taskType : taskTypes) {
- enabledTableConfigMap.put(taskType, new ArrayList<>());
-
- // Ensure all task queues exist
- _helixTaskResourceManager.ensureTaskQueueExists(taskType);
- }
+ Map<String, List<TableConfig>> enabledTableConfigMap = new HashMap<>();
// Scan all table configs to get the tables with tasks enabled
for (String tableNameWithType : tableNamesWithType) {
TableConfig tableConfig = _pinotHelixResourceManager.getTableConfig(tableNameWithType);
- if (tableConfig != null) {
- TableTaskConfig taskConfig = tableConfig.getTaskConfig();
- if (taskConfig != null) {
- for (String taskType : taskTypes) {
- if (taskConfig.isTaskTypeEnabled(taskType)) {
- enabledTableConfigMap.get(taskType).add(tableConfig);
- }
+ if (tableConfig != null && tableConfig.getTaskConfig() != null) {
+ Set<String> enabledTaskTypes = tableConfig.getTaskConfig().getTaskTypeConfigsMap().keySet();
+ for (String enabledTaskType : enabledTaskTypes) {
+ if (taskTypes.contains(enabledTaskType)) {
+ enabledTableConfigMap.computeIfAbsent(enabledTaskType, k -> new ArrayList<>()).add(tableConfig);
+ } else {
+ LOGGER.warn("Task type: {} is not registered, cannot enable it for table: {}", enabledTaskType,
Review comment:
Good point. We cannot directly return the message without backward-incompatible change on the rest endpoint (currently returns the map from task type to task scheduled). I made the change so that the returned map contains all the task types within the table configs, and if the task type is not registered, there will be no task scheduled (value is `null` for the task type), and then user can look into the log and find the reason.
----------------------------------------------------------------
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.
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
[GitHub] [incubator-pinot] fx19880617 commented on a change in pull request #6352: Enhance task schedule api for single type/table support
Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #6352:
URL: https://github.com/apache/incubator-pinot/pull/6352#discussion_r543958481
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java
##########
@@ -63,99 +65,147 @@ public PinotTaskManager(PinotHelixTaskResourceManager helixTaskResourceManager,
}
/**
- * Returns the cluster info provider.
- * <p>
- * Cluster info provider might be useful when initializing task generators.
- *
- * @return Cluster info provider
+ * Returns the cluster info accessor.
+ * <p>Cluster info accessor can be used to initialize the task generator.
*/
public ClusterInfoAccessor getClusterInfoAccessor() {
return _clusterInfoAccessor;
}
/**
* Registers a task generator.
- * <p>
- * This method can be used to plug in custom task generators.
- *
- * @param pinotTaskGenerator Task generator to be registered
+ * <p>This method can be used to plug in custom task generators.
*/
- public void registerTaskGenerator(PinotTaskGenerator pinotTaskGenerator) {
- _taskGeneratorRegistry.registerTaskGenerator(pinotTaskGenerator);
+ public void registerTaskGenerator(PinotTaskGenerator taskGenerator) {
+ _taskGeneratorRegistry.registerTaskGenerator(taskGenerator);
}
/**
- * Public API to schedule tasks. It doesn't matter whether current pinot controller is leader.
+ * Public API to schedule tasks (all task types) for all tables. It might be called from the non-leader controller.
+ * Returns a map from the task type to the task scheduled.
*/
public synchronized Map<String, String> scheduleTasks() {
- Map<String, String> tasksScheduled = scheduleTasks(_pinotHelixResourceManager.getAllTables());
-
- // Reset the task because this method will be called from the Rest API instead of the periodic task scheduler
- // TODO: Clean up only the non-leader tables instead of all tables
- cleanUpTask();
- setUpTask();
-
- return tasksScheduled;
+ return scheduleTasks(_pinotHelixResourceManager.getAllTables(), false);
Review comment:
why this is always false?
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/SimpleMinionClusterIntegrationTest.java
##########
@@ -48,10 +48,7 @@
import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;
-import static org.testng.Assert.assertEquals;
-import static org.testng.Assert.assertFalse;
-import static org.testng.Assert.assertNotNull;
-import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.*;
Review comment:
expand the imports
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java
##########
@@ -63,99 +65,147 @@ public PinotTaskManager(PinotHelixTaskResourceManager helixTaskResourceManager,
}
/**
- * Returns the cluster info provider.
- * <p>
- * Cluster info provider might be useful when initializing task generators.
- *
- * @return Cluster info provider
+ * Returns the cluster info accessor.
+ * <p>Cluster info accessor can be used to initialize the task generator.
*/
public ClusterInfoAccessor getClusterInfoAccessor() {
return _clusterInfoAccessor;
}
/**
* Registers a task generator.
- * <p>
- * This method can be used to plug in custom task generators.
- *
- * @param pinotTaskGenerator Task generator to be registered
+ * <p>This method can be used to plug in custom task generators.
*/
- public void registerTaskGenerator(PinotTaskGenerator pinotTaskGenerator) {
- _taskGeneratorRegistry.registerTaskGenerator(pinotTaskGenerator);
+ public void registerTaskGenerator(PinotTaskGenerator taskGenerator) {
+ _taskGeneratorRegistry.registerTaskGenerator(taskGenerator);
}
/**
- * Public API to schedule tasks. It doesn't matter whether current pinot controller is leader.
+ * Public API to schedule tasks (all task types) for all tables. It might be called from the non-leader controller.
+ * Returns a map from the task type to the task scheduled.
*/
public synchronized Map<String, String> scheduleTasks() {
- Map<String, String> tasksScheduled = scheduleTasks(_pinotHelixResourceManager.getAllTables());
-
- // Reset the task because this method will be called from the Rest API instead of the periodic task scheduler
- // TODO: Clean up only the non-leader tables instead of all tables
- cleanUpTask();
- setUpTask();
-
- return tasksScheduled;
+ return scheduleTasks(_pinotHelixResourceManager.getAllTables(), false);
}
/**
- * Check the Pinot cluster status and schedule new tasks for the given tables.
- *
- * @param tableNamesWithType List of table names with type suffix
- * @return Map from task type to task scheduled
+ * 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 task scheduled.
*/
- private synchronized Map<String, String> scheduleTasks(List<String> tableNamesWithType) {
+ private synchronized Map<String, String> scheduleTasks(List<String> tableNamesWithType, boolean isLeader) {
_controllerMetrics.addMeteredGlobalValue(ControllerMeter.NUMBER_TIMES_SCHEDULE_TASKS_CALLED, 1L);
Set<String> taskTypes = _taskGeneratorRegistry.getAllTaskTypes();
- int numTaskTypes = taskTypes.size();
- Map<String, List<TableConfig>> enabledTableConfigMap = new HashMap<>(numTaskTypes);
-
- for (String taskType : taskTypes) {
- enabledTableConfigMap.put(taskType, new ArrayList<>());
-
- // Ensure all task queues exist
- _helixTaskResourceManager.ensureTaskQueueExists(taskType);
- }
+ Map<String, List<TableConfig>> enabledTableConfigMap = new HashMap<>();
// Scan all table configs to get the tables with tasks enabled
for (String tableNameWithType : tableNamesWithType) {
TableConfig tableConfig = _pinotHelixResourceManager.getTableConfig(tableNameWithType);
- if (tableConfig != null) {
- TableTaskConfig taskConfig = tableConfig.getTaskConfig();
- if (taskConfig != null) {
- for (String taskType : taskTypes) {
- if (taskConfig.isTaskTypeEnabled(taskType)) {
- enabledTableConfigMap.get(taskType).add(tableConfig);
- }
+ if (tableConfig != null && tableConfig.getTaskConfig() != null) {
+ Set<String> enabledTaskTypes = tableConfig.getTaskConfig().getTaskTypeConfigsMap().keySet();
+ for (String enabledTaskType : enabledTaskTypes) {
+ if (taskTypes.contains(enabledTaskType)) {
+ enabledTableConfigMap.computeIfAbsent(enabledTaskType, k -> new ArrayList<>()).add(tableConfig);
+ } else {
+ LOGGER.warn("Task type: {} is not registered, cannot enable it for table: {}", enabledTaskType,
Review comment:
Is it possible that we return this info to the client-side? I feel this information is useful for users to test and debug.
----------------------------------------------------------------
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.
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
[GitHub] [incubator-pinot] fx19880617 commented on a change in pull request #6352: Enhance task schedule api for single type/table support
Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #6352:
URL: https://github.com/apache/incubator-pinot/pull/6352#discussion_r544643264
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java
##########
@@ -63,99 +65,147 @@ public PinotTaskManager(PinotHelixTaskResourceManager helixTaskResourceManager,
}
/**
- * Returns the cluster info provider.
- * <p>
- * Cluster info provider might be useful when initializing task generators.
- *
- * @return Cluster info provider
+ * Returns the cluster info accessor.
+ * <p>Cluster info accessor can be used to initialize the task generator.
*/
public ClusterInfoAccessor getClusterInfoAccessor() {
return _clusterInfoAccessor;
}
/**
* Registers a task generator.
- * <p>
- * This method can be used to plug in custom task generators.
- *
- * @param pinotTaskGenerator Task generator to be registered
+ * <p>This method can be used to plug in custom task generators.
*/
- public void registerTaskGenerator(PinotTaskGenerator pinotTaskGenerator) {
- _taskGeneratorRegistry.registerTaskGenerator(pinotTaskGenerator);
+ public void registerTaskGenerator(PinotTaskGenerator taskGenerator) {
+ _taskGeneratorRegistry.registerTaskGenerator(taskGenerator);
}
/**
- * Public API to schedule tasks. It doesn't matter whether current pinot controller is leader.
+ * Public API to schedule tasks (all task types) for all tables. It might be called from the non-leader controller.
+ * Returns a map from the task type to the task scheduled.
*/
public synchronized Map<String, String> scheduleTasks() {
- Map<String, String> tasksScheduled = scheduleTasks(_pinotHelixResourceManager.getAllTables());
-
- // Reset the task because this method will be called from the Rest API instead of the periodic task scheduler
- // TODO: Clean up only the non-leader tables instead of all tables
- cleanUpTask();
- setUpTask();
-
- return tasksScheduled;
+ return scheduleTasks(_pinotHelixResourceManager.getAllTables(), false);
}
/**
- * Check the Pinot cluster status and schedule new tasks for the given tables.
- *
- * @param tableNamesWithType List of table names with type suffix
- * @return Map from task type to task scheduled
+ * 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 task scheduled.
*/
- private synchronized Map<String, String> scheduleTasks(List<String> tableNamesWithType) {
+ private synchronized Map<String, String> scheduleTasks(List<String> tableNamesWithType, boolean isLeader) {
_controllerMetrics.addMeteredGlobalValue(ControllerMeter.NUMBER_TIMES_SCHEDULE_TASKS_CALLED, 1L);
Set<String> taskTypes = _taskGeneratorRegistry.getAllTaskTypes();
- int numTaskTypes = taskTypes.size();
- Map<String, List<TableConfig>> enabledTableConfigMap = new HashMap<>(numTaskTypes);
-
- for (String taskType : taskTypes) {
- enabledTableConfigMap.put(taskType, new ArrayList<>());
-
- // Ensure all task queues exist
- _helixTaskResourceManager.ensureTaskQueueExists(taskType);
- }
+ Map<String, List<TableConfig>> enabledTableConfigMap = new HashMap<>();
// Scan all table configs to get the tables with tasks enabled
for (String tableNameWithType : tableNamesWithType) {
TableConfig tableConfig = _pinotHelixResourceManager.getTableConfig(tableNameWithType);
- if (tableConfig != null) {
- TableTaskConfig taskConfig = tableConfig.getTaskConfig();
- if (taskConfig != null) {
- for (String taskType : taskTypes) {
- if (taskConfig.isTaskTypeEnabled(taskType)) {
- enabledTableConfigMap.get(taskType).add(tableConfig);
- }
+ if (tableConfig != null && tableConfig.getTaskConfig() != null) {
+ Set<String> enabledTaskTypes = tableConfig.getTaskConfig().getTaskTypeConfigsMap().keySet();
+ for (String enabledTaskType : enabledTaskTypes) {
+ if (taskTypes.contains(enabledTaskType)) {
+ enabledTableConfigMap.computeIfAbsent(enabledTaskType, k -> new ArrayList<>()).add(tableConfig);
+ } else {
+ LOGGER.warn("Task type: {} is not registered, cannot enable it for table: {}", enabledTaskType,
Review comment:
true, maybe we add a new array field like "errors" later to hold these information.
----------------------------------------------------------------
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.
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
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6352: Enhance task schedule api for single type/table support
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6352:
URL: https://github.com/apache/incubator-pinot/pull/6352#discussion_r544570603
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java
##########
@@ -63,99 +65,147 @@ public PinotTaskManager(PinotHelixTaskResourceManager helixTaskResourceManager,
}
/**
- * Returns the cluster info provider.
- * <p>
- * Cluster info provider might be useful when initializing task generators.
- *
- * @return Cluster info provider
+ * Returns the cluster info accessor.
+ * <p>Cluster info accessor can be used to initialize the task generator.
*/
public ClusterInfoAccessor getClusterInfoAccessor() {
return _clusterInfoAccessor;
}
/**
* Registers a task generator.
- * <p>
- * This method can be used to plug in custom task generators.
- *
- * @param pinotTaskGenerator Task generator to be registered
+ * <p>This method can be used to plug in custom task generators.
*/
- public void registerTaskGenerator(PinotTaskGenerator pinotTaskGenerator) {
- _taskGeneratorRegistry.registerTaskGenerator(pinotTaskGenerator);
+ public void registerTaskGenerator(PinotTaskGenerator taskGenerator) {
+ _taskGeneratorRegistry.registerTaskGenerator(taskGenerator);
}
/**
- * Public API to schedule tasks. It doesn't matter whether current pinot controller is leader.
+ * Public API to schedule tasks (all task types) for all tables. It might be called from the non-leader controller.
+ * Returns a map from the task type to the task scheduled.
*/
public synchronized Map<String, String> scheduleTasks() {
- Map<String, String> tasksScheduled = scheduleTasks(_pinotHelixResourceManager.getAllTables());
-
- // Reset the task because this method will be called from the Rest API instead of the periodic task scheduler
- // TODO: Clean up only the non-leader tables instead of all tables
- cleanUpTask();
- setUpTask();
-
- return tasksScheduled;
+ return scheduleTasks(_pinotHelixResourceManager.getAllTables(), false);
Review comment:
This method is connected to the rest endpoint, so the caller might not be the leader controller
----------------------------------------------------------------
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.
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