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 2022/10/05 22:21:18 UTC
[GitHub] [pinot] klsince opened a new pull request, #9540: get task runtime configs tracked in Helix
klsince opened a new pull request, #9540:
URL: https://github.com/apache/pinot/pull/9540
Added a restful API to get task runtime configs like concurrencyPerWorker and taskTimeout. Note that those are not inside the subtasks' configs. Also refine some logs found useful when debugging minion related APIs.
--
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
[GitHub] [pinot] klsince commented on a diff in pull request #9540: get task runtime configs tracked in Helix
Posted by GitBox <gi...@apache.org>.
klsince commented on code in PR #9540:
URL: https://github.com/apache/pinot/pull/9540#discussion_r988428508
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTaskRestletResource.java:
##########
@@ -386,7 +386,15 @@ public Map<String, TaskPartitionState> getSubtaskStates(
@ApiOperation("Get the task config (a list of child task configs) for the given task")
public List<PinotTaskConfig> getTaskConfigs(
@ApiParam(value = "Task name", required = true) @PathParam("taskName") String taskName) {
- return _pinotHelixTaskResourceManager.getTaskConfigs(taskName);
+ return _pinotHelixTaskResourceManager.getSubtaskConfigs(taskName);
+ }
+
+ @GET
+ @Path("/tasks/task/{taskName}/runtime/config")
+ @ApiOperation("Get the task runtime config for the given task")
+ public Map<String, String> getTaskConfig(
Review Comment:
I'd prefer a separate API, in case any applications are assuming a List<PinotTaskConfig> returned by the old API. (if we have versioned our APIs with prefix like v1/v2, I'd save a lot of time to come up with a new name :p)
--
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
[GitHub] [pinot] zhtaoxiang commented on a diff in pull request #9540: get task runtime configs tracked in Helix
Posted by GitBox <gi...@apache.org>.
zhtaoxiang commented on code in PR #9540:
URL: https://github.com/apache/pinot/pull/9540#discussion_r988424450
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotHelixTaskResourceManager.java:
##########
@@ -434,7 +434,7 @@ public synchronized Map<String, TaskPartitionState> getSubtaskStates(String task
* @param taskName Task name
* @return List of child task configs
*/
- public synchronized List<PinotTaskConfig> getTaskConfigs(String taskName) {
+ public synchronized List<PinotTaskConfig> getSubtaskConfigs(String taskName) {
Review Comment:
Could you please help me understand why we change the name? It seems to me that Helix task is the right concept?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTaskRestletResource.java:
##########
@@ -386,7 +386,15 @@ public Map<String, TaskPartitionState> getSubtaskStates(
@ApiOperation("Get the task config (a list of child task configs) for the given task")
public List<PinotTaskConfig> getTaskConfigs(
@ApiParam(value = "Task name", required = true) @PathParam("taskName") String taskName) {
- return _pinotHelixTaskResourceManager.getTaskConfigs(taskName);
+ return _pinotHelixTaskResourceManager.getSubtaskConfigs(taskName);
+ }
+
+ @GET
+ @Path("/tasks/task/{taskName}/runtime/config")
+ @ApiOperation("Get the task runtime config for the given task")
+ public Map<String, String> getTaskConfig(
Review Comment:
just another thought: is it better to add a separate endpoint or is it better to include more information in the existing endpoint?
--
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
[GitHub] [pinot] klsince commented on a diff in pull request #9540: get task runtime configs tracked in Helix
Posted by GitBox <gi...@apache.org>.
klsince commented on code in PR #9540:
URL: https://github.com/apache/pinot/pull/9540#discussion_r989449794
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotHelixTaskResourceManager.java:
##########
@@ -444,6 +444,24 @@ public synchronized List<PinotTaskConfig> getTaskConfigs(String taskName) {
return taskConfigs;
}
+ /**
+ * Get the task runtime config for the given task name. A task can have multiple subtasks, whose configs can be
+ * retrieved via the getSubtaskConfigs() method instead.
+ *
+ * @param taskName Task name
+ * @return Configs for the task returned as a Map.
+ */
+ public synchronized Map<String, String> getTaskRuntimeConfig(String taskName) {
+ JobConfig jobConfig = _taskDriver.getJobConfig(getHelixJobName(taskName));
+ jobConfig.getNumConcurrentTasksPerInstance();
Review Comment:
sharp eye
--
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
[GitHub] [pinot] codecov-commenter commented on pull request #9540: get task runtime configs tracked in Helix
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9540:
URL: https://github.com/apache/pinot/pull/9540#issuecomment-1269096987
# [Codecov](https://codecov.io/gh/apache/pinot/pull/9540?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#9540](https://codecov.io/gh/apache/pinot/pull/9540?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7fbcf64) into [master](https://codecov.io/gh/apache/pinot/commit/c145ac853b10d1a5b3d7c9038bac4d7d4d0bf3a5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c145ac8) will **decrease** coverage by `6.16%`.
> The diff coverage is `8.69%`.
```diff
@@ Coverage Diff @@
## master #9540 +/- ##
============================================
- Coverage 69.86% 63.69% -6.17%
- Complexity 4795 4848 +53
============================================
Files 1927 1874 -53
Lines 102697 100401 -2296
Branches 15586 15317 -269
============================================
- Hits 71747 63951 -7796
- Misses 25878 31741 +5863
+ Partials 5072 4709 -363
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `67.26% <ø> (+0.04%)` | :arrow_up: |
| unittests2 | `15.59% <8.69%> (+0.04%)` | :arrow_up: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/9540?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...roller/api/resources/PinotTaskRestletResource.java](https://codecov.io/gh/apache/pinot/pull/9540/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90VGFza1Jlc3RsZXRSZXNvdXJjZS5qYXZh) | `0.00% <0.00%> (-2.99%)` | :arrow_down: |
| [...troller/helix/core/minion/ClusterInfoAccessor.java](https://codecov.io/gh/apache/pinot/pull/9540/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL21pbmlvbi9DbHVzdGVySW5mb0FjY2Vzc29yLmphdmE=) | `37.93% <0.00%> (-41.38%)` | :arrow_down: |
| [...lix/core/minion/PinotHelixTaskResourceManager.java](https://codecov.io/gh/apache/pinot/pull/9540/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL21pbmlvbi9QaW5vdEhlbGl4VGFza1Jlc291cmNlTWFuYWdlci5qYXZh) | `14.03% <0.00%> (-28.05%)` | :arrow_down: |
| [...pache/pinot/minion/event/MinionEventObservers.java](https://codecov.io/gh/apache/pinot/pull/9540/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXZlbnQvTWluaW9uRXZlbnRPYnNlcnZlcnMuamF2YQ==) | `87.09% <40.00%> (-2.56%)` | :arrow_down: |
| [...va/org/apache/pinot/core/routing/RoutingTable.java](https://codecov.io/gh/apache/pinot/pull/9540/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yb3V0aW5nL1JvdXRpbmdUYWJsZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...va/org/apache/pinot/common/config/NettyConfig.java](https://codecov.io/gh/apache/pinot/pull/9540/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL05ldHR5Q29uZmlnLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/common/metrics/MinionMeter.java](https://codecov.io/gh/apache/pinot/pull/9540/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...g/apache/pinot/common/metrics/ControllerMeter.java](https://codecov.io/gh/apache/pinot/pull/9540/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Db250cm9sbGVyTWV0ZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/pinot/pull/9540/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/pinot/pull/9540/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [433 more](https://codecov.io/gh/apache/pinot/pull/9540/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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
[GitHub] [pinot] klsince commented on a diff in pull request #9540: get task runtime configs tracked in Helix
Posted by GitBox <gi...@apache.org>.
klsince commented on code in PR #9540:
URL: https://github.com/apache/pinot/pull/9540#discussion_r988432057
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotHelixTaskResourceManager.java:
##########
@@ -434,7 +434,7 @@ public synchronized Map<String, TaskPartitionState> getSubtaskStates(String task
* @param taskName Task name
* @return List of child task configs
*/
- public synchronized List<PinotTaskConfig> getTaskConfigs(String taskName) {
+ public synchronized List<PinotTaskConfig> getSubtaskConfigs(String taskName) {
Review Comment:
I assume methods in this class are defined in terms of the abstraction of `Minion task` (not `Helix job`, which we use to model a minion task). A minion task can have multiple subtasks, and as said in this method comment, it is to `Get the child task configs for the given task name`. So I renamed it.
If keeping the name of this method, I'd call the new method getJobConfig(), but we don't expose the `job` concept in the minion framework in most of the implementations to my best knowledge. I do see places task/subtask are used exchangeably and I had to understand the context to figure out which is which.
--
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
[GitHub] [pinot] Jackie-Jiang merged pull request #9540: get task runtime configs tracked in Helix
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged PR #9540:
URL: https://github.com/apache/pinot/pull/9540
--
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
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #9540: get task runtime configs tracked in Helix
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #9540:
URL: https://github.com/apache/pinot/pull/9540#discussion_r989355875
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotHelixTaskResourceManager.java:
##########
@@ -444,6 +444,24 @@ public synchronized List<PinotTaskConfig> getTaskConfigs(String taskName) {
return taskConfigs;
}
+ /**
+ * Get the task runtime config for the given task name. A task can have multiple subtasks, whose configs can be
+ * retrieved via the getSubtaskConfigs() method instead.
+ *
+ * @param taskName Task name
+ * @return Configs for the task returned as a Map.
+ */
+ public synchronized Map<String, String> getTaskRuntimeConfig(String taskName) {
+ JobConfig jobConfig = _taskDriver.getJobConfig(getHelixJobName(taskName));
+ jobConfig.getNumConcurrentTasksPerInstance();
Review Comment:
Redundant?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotHelixTaskResourceManager.java:
##########
@@ -444,6 +444,24 @@ public synchronized List<PinotTaskConfig> getTaskConfigs(String taskName) {
return taskConfigs;
}
+ /**
+ * Get the task runtime config for the given task name. A task can have multiple subtasks, whose configs can be
+ * retrieved via the getSubtaskConfigs() method instead.
+ *
+ * @param taskName Task name
+ * @return Configs for the task returned as a Map.
+ */
+ public synchronized Map<String, String> getTaskRuntimeConfig(String taskName) {
+ JobConfig jobConfig = _taskDriver.getJobConfig(getHelixJobName(taskName));
+ jobConfig.getNumConcurrentTasksPerInstance();
+ HashMap<String, String> configs = new HashMap<>();
+ configs.put("ConcurrentTasksPerWorker", String.valueOf(jobConfig.getNumConcurrentTasksPerInstance()));
+ configs.put("TaskTimeoutInMs", String.valueOf(jobConfig.getTimeoutPerTask()));
+ configs.put("TaskExpireTimeInMs", String.valueOf(jobConfig.getExpiry()));
Review Comment:
(minor) We usually not put `In`
```suggestion
configs.put("TaskTimeoutMs", String.valueOf(jobConfig.getTimeoutPerTask()));
configs.put("TaskExpireTimeMs", String.valueOf(jobConfig.getExpiry()));
```
--
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