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/09/22 21:02:35 UTC
[GitHub] [pinot] klsince opened a new pull request, #9449: refine minion worker event observer to track finer grained progress for tasks
klsince opened a new pull request, #9449:
URL: https://github.com/apache/pinot/pull/9449
This PR adds upon the last one #9432 to track finer grained task progress for current minion task types, and has refined the progress observer to keep last N status to be more informative.
--
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 #9449: refine minion worker event observer to track finer grained progress for tasks
Posted by GitBox <gi...@apache.org>.
klsince commented on code in PR #9449:
URL: https://github.com/apache/pinot/pull/9449#discussion_r978157562
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/realtimetoofflinesegments/RealtimeToOfflineSegmentsTaskExecutor.java:
##########
@@ -111,6 +111,8 @@ public void preProcess(PinotTaskConfig pinotTaskConfig) {
protected List<SegmentConversionResult> convert(PinotTaskConfig pinotTaskConfig, List<File> segmentDirs,
File workingDir)
throws Exception {
+ int numInputSegments = segmentDirs.size();
+ _eventObserver.notifyProgress(pinotTaskConfig, "Converting segments: " + numInputSegments);
Review Comment:
this is a subclass, and the segment name is tracked by caller of convert().
--
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] npawar merged pull request #9449: refine minion worker event observer to track finer grained progress for tasks
Posted by GitBox <gi...@apache.org>.
npawar merged PR #9449:
URL: https://github.com/apache/pinot/pull/9449
--
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 #9449: refine minion worker event observer to track finer grained progress for tasks
Posted by GitBox <gi...@apache.org>.
klsince commented on code in PR #9449:
URL: https://github.com/apache/pinot/pull/9449#discussion_r978160605
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/converttorawindex/ConvertToRowIndexTaskProgressObserverFactory.java:
##########
@@ -0,0 +1,45 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.plugin.minion.tasks.converttorawindex;
+
+import org.apache.pinot.core.common.MinionConstants;
+import org.apache.pinot.minion.event.MinionEventObserver;
+import org.apache.pinot.minion.event.MinionEventObserverFactory;
+import org.apache.pinot.minion.event.MinionProgressObserver;
+import org.apache.pinot.minion.executor.MinionTaskZkMetadataManager;
+import org.apache.pinot.spi.annotations.minion.EventObserverFactory;
+
+
+@EventObserverFactory
+public class ConvertToRowIndexTaskProgressObserverFactory implements MinionEventObserverFactory {
Review Comment:
yup
--
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 #9449: refine minion worker event observer to track finer grained progress for tasks
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9449:
URL: https://github.com/apache/pinot/pull/9449#issuecomment-1255605710
# [Codecov](https://codecov.io/gh/apache/pinot/pull/9449?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 [#9449](https://codecov.io/gh/apache/pinot/pull/9449?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (eeefc0c) into [master](https://codecov.io/gh/apache/pinot/commit/7af0d20aaa9e8ec7ad652a0209457cd226a2b0f0?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7af0d20) will **decrease** coverage by `43.77%`.
> The diff coverage is `77.01%`.
```diff
@@ Coverage Diff @@
## master #9449 +/- ##
=============================================
- Coverage 69.85% 26.08% -43.78%
+ Complexity 5098 44 -5054
=============================================
Files 1902 1894 -8
Lines 101517 101289 -228
Branches 15411 15387 -24
=============================================
- Hits 70917 26420 -44497
- Misses 25596 72238 +46642
+ Partials 5004 2631 -2373
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `26.08% <77.01%> (+0.05%)` | :arrow_up: |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `?` | |
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/9449?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/9449/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) | `2.98% <0.00%> (-0.02%)` | :arrow_down: |
| [...lix/core/minion/PinotHelixTaskResourceManager.java](https://codecov.io/gh/apache/pinot/pull/9449/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) | `30.52% <0.00%> (-9.22%)` | :arrow_down: |
| [...inion/api/resources/PinotTaskProgressResource.java](https://codecov.io/gh/apache/pinot/pull/9449/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-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vYXBpL3Jlc291cmNlcy9QaW5vdFRhc2tQcm9ncmVzc1Jlc291cmNlLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...t/plugin/minion/tasks/purge/PurgeTaskExecutor.java](https://codecov.io/gh/apache/pinot/pull/9449/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-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3MvcHVyZ2UvUHVyZ2VUYXNrRXhlY3V0b3IuamF2YQ==) | `0.00% <0.00%> (-90.91%)` | :arrow_down: |
| [...nandpush/SegmentGenerationAndPushTaskExecutor.java](https://codecov.io/gh/apache/pinot/pull/9449/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-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3Mvc2VnbWVudGdlbmVyYXRpb25hbmRwdXNoL1NlZ21lbnRHZW5lcmF0aW9uQW5kUHVzaFRhc2tFeGVjdXRvci5qYXZh) | `66.90% <ø> (-0.24%)` | :arrow_down: |
| [...pache/pinot/minion/event/MinionEventObservers.java](https://codecov.io/gh/apache/pinot/pull/9449/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==) | `44.82% <50.00%> (-46.56%)` | :arrow_down: |
| [...che/pinot/minion/event/MinionProgressObserver.java](https://codecov.io/gh/apache/pinot/pull/9449/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-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXZlbnQvTWluaW9uUHJvZ3Jlc3NPYnNlcnZlci5qYXZh) | `52.27% <60.86%> (+7.82%)` | :arrow_up: |
| [.../tasks/purge/PurgeTaskProgressObserverFactory.java](https://codecov.io/gh/apache/pinot/pull/9449/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-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3MvcHVyZ2UvUHVyZ2VUYXNrUHJvZ3Jlc3NPYnNlcnZlckZhY3RvcnkuamF2YQ==) | `75.00% <75.00%> (ø)` | |
| [.../tasks/BaseMultipleSegmentsConversionExecutor.java](https://codecov.io/gh/apache/pinot/pull/9449/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-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3MvQmFzZU11bHRpcGxlU2VnbWVudHNDb252ZXJzaW9uRXhlY3V0b3IuamF2YQ==) | `91.17% <100.00%> (+1.17%)` | :arrow_up: |
| [...ion/tasks/BaseSingleSegmentConversionExecutor.java](https://codecov.io/gh/apache/pinot/pull/9449/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-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3MvQmFzZVNpbmdsZVNlZ21lbnRDb252ZXJzaW9uRXhlY3V0b3IuamF2YQ==) | `77.46% <100.00%> (+2.08%)` | :arrow_up: |
| ... and [1393 more](https://codecov.io/gh/apache/pinot/pull/9449/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 #9449: refine minion worker event observer to track finer grained progress for tasks
Posted by GitBox <gi...@apache.org>.
klsince commented on code in PR #9449:
URL: https://github.com/apache/pinot/pull/9449#discussion_r978153285
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTaskRestletResource.java:
##########
@@ -410,8 +410,9 @@ public Map<String, PinotTaskConfig> getSubtaskConfigs(
@GET
@Path("/tasks/subtask/{taskName}/progress")
+ @Produces(MediaType.APPLICATION_JSON)
@ApiOperation("Get progress of specified sub tasks for the given task tracked by worker in memory")
- public Map<String, String> getSubtaskProgress(@Context HttpHeaders httpHeaders,
+ public String getSubtaskProgress(@Context HttpHeaders httpHeaders,
Review Comment:
I don't think there is use of this, as it's added yesterday in my last PR. The change here should make it more flexible to be extended later on.
--
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 #9449: refine minion worker event observer to track finer grained progress for tasks
Posted by GitBox <gi...@apache.org>.
klsince commented on code in PR #9449:
URL: https://github.com/apache/pinot/pull/9449#discussion_r978153648
##########
pinot-minion/src/main/java/org/apache/pinot/minion/api/resources/PinotTaskProgressResource.java:
##########
@@ -57,23 +60,24 @@ public class PinotTaskProgressResource {
@GET
@Path("/tasks/subtask/progress")
+ @Produces(MediaType.APPLICATION_JSON)
@ApiOperation("Get finer grained task progress tracked in memory for the given subtasks")
@ApiResponses(value = {
@ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 500, message = "Internal server error")
})
- public Object getSubtaskProgress(
+ public String getSubtaskProgress(
Review Comment:
replied above
--
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 #9449: refine minion worker event observer to track finer grained progress for tasks
Posted by GitBox <gi...@apache.org>.
klsince commented on code in PR #9449:
URL: https://github.com/apache/pinot/pull/9449#discussion_r978256733
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/BaseMultipleSegmentsConversionExecutor.java:
##########
@@ -206,6 +232,8 @@ public List<SegmentConversionResult> executeTask(PinotTaskConfig pinotTaskConfig
File convertedTarredSegmentFile = tarredSegmentFiles.get(i);
SegmentConversionResult segmentConversionResult = segmentConversionResults.get(i);
String resultSegmentName = segmentConversionResult.getSegmentName();
+ _eventObserver.notifyProgress(_pinotTaskConfig,
+ String.format("Uploading segment: %s, %d/%d", resultSegmentName, count++, numOutputSegments));
Review Comment:
sharp eye. thx
--
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] jackjlli commented on a diff in pull request #9449: refine minion worker event observer to track finer grained progress for tasks
Posted by GitBox <gi...@apache.org>.
jackjlli commented on code in PR #9449:
URL: https://github.com/apache/pinot/pull/9449#discussion_r978178337
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/BaseMultipleSegmentsConversionExecutor.java:
##########
@@ -206,6 +232,8 @@ public List<SegmentConversionResult> executeTask(PinotTaskConfig pinotTaskConfig
File convertedTarredSegmentFile = tarredSegmentFiles.get(i);
SegmentConversionResult segmentConversionResult = segmentConversionResults.get(i);
String resultSegmentName = segmentConversionResult.getSegmentName();
+ _eventObserver.notifyProgress(_pinotTaskConfig,
+ String.format("Uploading segment: %s, %d/%d", resultSegmentName, count++, numOutputSegments));
Review Comment:
Using `count` here seems incorrect. It'd be good to use `i + 1`?
--
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 #9449: refine minion worker event observer to track finer grained progress for tasks
Posted by GitBox <gi...@apache.org>.
zhtaoxiang commented on code in PR #9449:
URL: https://github.com/apache/pinot/pull/9449#discussion_r978120634
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/realtimetoofflinesegments/RealtimeToOfflineSegmentsTaskExecutor.java:
##########
@@ -111,6 +111,8 @@ public void preProcess(PinotTaskConfig pinotTaskConfig) {
protected List<SegmentConversionResult> convert(PinotTaskConfig pinotTaskConfig, List<File> segmentDirs,
File workingDir)
throws Exception {
+ int numInputSegments = segmentDirs.size();
+ _eventObserver.notifyProgress(pinotTaskConfig, "Converting segments: " + numInputSegments);
Review Comment:
maybe we can also include segment names?
##########
pinot-minion/src/main/java/org/apache/pinot/minion/event/MinionProgressObserver.java:
##########
@@ -29,57 +34,100 @@
/**
* A minion event observer that can track task progress status in memory.
*/
+@ThreadSafe
public class MinionProgressObserver extends DefaultMinionEventObserver {
private static final Logger LOGGER = LoggerFactory.getLogger(MinionProgressObserver.class);
+ // TODO: make this configurable
+ private static final int DEFAULT_MAX_NUM_STATUS_TO_TRACK = 128;
- private static volatile long _startTs;
- private static volatile Object _lastStatus;
+ private final int _maxNumStatusToTrack;
+ private final Deque<StatusEntry> _lastStatus = new LinkedList<>();
+ private long _startTs;
+
+ public MinionProgressObserver() {
+ this(DEFAULT_MAX_NUM_STATUS_TO_TRACK);
+ }
+
+ public MinionProgressObserver(int maxNumStatusToTrack) {
+ _maxNumStatusToTrack = maxNumStatusToTrack;
+ }
@Override
- public void notifyTaskStart(PinotTaskConfig pinotTaskConfig) {
+ public synchronized void notifyTaskStart(PinotTaskConfig pinotTaskConfig) {
_startTs = System.currentTimeMillis();
+ addStatus(_startTs, "Task started");
super.notifyTaskStart(pinotTaskConfig);
}
- public void notifyProgress(PinotTaskConfig pinotTaskConfig, @Nullable Object progress) {
+ public synchronized void notifyProgress(PinotTaskConfig pinotTaskConfig, @Nullable Object progress) {
if (LOGGER.isDebugEnabled()) {
LOGGER.debug("Update progress: {} for task: {}", progress, pinotTaskConfig.getTaskId());
}
- _lastStatus = progress;
+ addStatus(System.currentTimeMillis(), (progress == null) ? "" : progress.toString());
Review Comment:
This assumes that `progress.toString()` can always produce meaningful string, is this a valid assumption?
##########
pinot-minion/src/main/java/org/apache/pinot/minion/api/resources/PinotTaskProgressResource.java:
##########
@@ -57,23 +60,24 @@ public class PinotTaskProgressResource {
@GET
@Path("/tasks/subtask/progress")
+ @Produces(MediaType.APPLICATION_JSON)
@ApiOperation("Get finer grained task progress tracked in memory for the given subtasks")
@ApiResponses(value = {
@ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 500, message = "Internal server error")
})
- public Object getSubtaskProgress(
+ public String getSubtaskProgress(
Review Comment:
ditto:this is not backward compatible, is it okay to keep the old one or add a new one?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTaskRestletResource.java:
##########
@@ -410,8 +410,9 @@ public Map<String, PinotTaskConfig> getSubtaskConfigs(
@GET
@Path("/tasks/subtask/{taskName}/progress")
+ @Produces(MediaType.APPLICATION_JSON)
@ApiOperation("Get progress of specified sub tasks for the given task tracked by worker in memory")
- public Map<String, String> getSubtaskProgress(@Context HttpHeaders httpHeaders,
+ public String getSubtaskProgress(@Context HttpHeaders httpHeaders,
Review Comment:
this is not backward compatible, is it okay to keep the old one or add a new one?
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/mergerollup/MergeRollupTaskExecutor.java:
##########
@@ -55,6 +55,8 @@ public MergeRollupTaskExecutor(MinionConf minionConf) {
protected List<SegmentConversionResult> convert(PinotTaskConfig pinotTaskConfig, List<File> segmentDirs,
File workingDir)
throws Exception {
+ int numInputSegments = segmentDirs.size();
+ _eventObserver.notifyProgress(pinotTaskConfig, "Converting segments: " + numInputSegments);
Review Comment:
should we include segment names?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotHelixTaskResourceManager.java:
##########
@@ -463,7 +463,7 @@ public synchronized Map<String, PinotTaskConfig> getSubtaskConfigs(String taskNa
return taskConfigs;
}
- public synchronized Map<String, String> getSubtaskProgress(String taskName, @Nullable String subtaskNames,
+ public synchronized Map<String, Object> getSubtaskProgress(String taskName, @Nullable String subtaskNames,
Review Comment:
This public method is not backward compatible and may affect other systems if they are using it, although in practice this might not be the case.
##########
pinot-minion/src/main/java/org/apache/pinot/minion/event/MinionProgressObserver.java:
##########
@@ -29,57 +34,100 @@
/**
* A minion event observer that can track task progress status in memory.
*/
+@ThreadSafe
public class MinionProgressObserver extends DefaultMinionEventObserver {
private static final Logger LOGGER = LoggerFactory.getLogger(MinionProgressObserver.class);
+ // TODO: make this configurable
+ private static final int DEFAULT_MAX_NUM_STATUS_TO_TRACK = 128;
- private static volatile long _startTs;
- private static volatile Object _lastStatus;
+ private final int _maxNumStatusToTrack;
+ private final Deque<StatusEntry> _lastStatus = new LinkedList<>();
+ private long _startTs;
+
+ public MinionProgressObserver() {
+ this(DEFAULT_MAX_NUM_STATUS_TO_TRACK);
+ }
+
+ public MinionProgressObserver(int maxNumStatusToTrack) {
+ _maxNumStatusToTrack = maxNumStatusToTrack;
+ }
@Override
- public void notifyTaskStart(PinotTaskConfig pinotTaskConfig) {
+ public synchronized void notifyTaskStart(PinotTaskConfig pinotTaskConfig) {
_startTs = System.currentTimeMillis();
+ addStatus(_startTs, "Task started");
super.notifyTaskStart(pinotTaskConfig);
}
- public void notifyProgress(PinotTaskConfig pinotTaskConfig, @Nullable Object progress) {
+ public synchronized void notifyProgress(PinotTaskConfig pinotTaskConfig, @Nullable Object progress) {
if (LOGGER.isDebugEnabled()) {
LOGGER.debug("Update progress: {} for task: {}", progress, pinotTaskConfig.getTaskId());
}
- _lastStatus = progress;
+ addStatus(System.currentTimeMillis(), (progress == null) ? "" : progress.toString());
super.notifyProgress(pinotTaskConfig, progress);
}
@Nullable
- public Object getProgress() {
- return _lastStatus;
+ public synchronized List<StatusEntry> getProgress() {
+ return new ArrayList<>(_lastStatus);
}
@Override
- public void notifyTaskSuccess(PinotTaskConfig pinotTaskConfig, @Nullable Object executionResult) {
+ public synchronized void notifyTaskSuccess(PinotTaskConfig pinotTaskConfig, @Nullable Object executionResult) {
long endTs = System.currentTimeMillis();
- _lastStatus = "Task succeeded in " + (endTs - _startTs) + "ms";
+ addStatus(endTs, "Task succeeded in " + (endTs - _startTs) + "ms");
super.notifyTaskSuccess(pinotTaskConfig, executionResult);
}
@Override
- public void notifyTaskCancelled(PinotTaskConfig pinotTaskConfig) {
+ public synchronized void notifyTaskCancelled(PinotTaskConfig pinotTaskConfig) {
long endTs = System.currentTimeMillis();
- _lastStatus = "Task got cancelled after " + (endTs - _startTs) + "ms";
+ addStatus(endTs, "Task got cancelled after " + (endTs - _startTs) + "ms");
super.notifyTaskCancelled(pinotTaskConfig);
}
@Override
- public void notifyTaskError(PinotTaskConfig pinotTaskConfig, Exception e) {
+ public synchronized void notifyTaskError(PinotTaskConfig pinotTaskConfig, Exception e) {
long endTs = System.currentTimeMillis();
- _lastStatus = "Task failed in " + (endTs - _startTs) + "ms, with error:\n" + makeStringFromException(e);
+ addStatus(endTs, "Task failed in " + (endTs - _startTs) + "ms with error: " + makeStringFromException(e));
super.notifyTaskError(pinotTaskConfig, e);
}
+ private void addStatus(long ts, String progress) {
Review Comment:
nit: maybe we can also add `synchronized` keyword here?
##########
pinot-minion/src/main/java/org/apache/pinot/minion/event/MinionProgressObserver.java:
##########
@@ -29,57 +34,100 @@
/**
* A minion event observer that can track task progress status in memory.
*/
+@ThreadSafe
public class MinionProgressObserver extends DefaultMinionEventObserver {
private static final Logger LOGGER = LoggerFactory.getLogger(MinionProgressObserver.class);
+ // TODO: make this configurable
+ private static final int DEFAULT_MAX_NUM_STATUS_TO_TRACK = 128;
- private static volatile long _startTs;
- private static volatile Object _lastStatus;
+ private final int _maxNumStatusToTrack;
+ private final Deque<StatusEntry> _lastStatus = new LinkedList<>();
+ private long _startTs;
+
+ public MinionProgressObserver() {
+ this(DEFAULT_MAX_NUM_STATUS_TO_TRACK);
+ }
+
+ public MinionProgressObserver(int maxNumStatusToTrack) {
+ _maxNumStatusToTrack = maxNumStatusToTrack;
+ }
@Override
- public void notifyTaskStart(PinotTaskConfig pinotTaskConfig) {
+ public synchronized void notifyTaskStart(PinotTaskConfig pinotTaskConfig) {
_startTs = System.currentTimeMillis();
+ addStatus(_startTs, "Task started");
super.notifyTaskStart(pinotTaskConfig);
}
- public void notifyProgress(PinotTaskConfig pinotTaskConfig, @Nullable Object progress) {
+ public synchronized void notifyProgress(PinotTaskConfig pinotTaskConfig, @Nullable Object progress) {
if (LOGGER.isDebugEnabled()) {
LOGGER.debug("Update progress: {} for task: {}", progress, pinotTaskConfig.getTaskId());
}
- _lastStatus = progress;
+ addStatus(System.currentTimeMillis(), (progress == null) ? "" : progress.toString());
super.notifyProgress(pinotTaskConfig, progress);
}
@Nullable
- public Object getProgress() {
- return _lastStatus;
+ public synchronized List<StatusEntry> getProgress() {
+ return new ArrayList<>(_lastStatus);
Review Comment:
how about using immutableList?
##########
pinot-minion/src/main/java/org/apache/pinot/minion/event/MinionProgressObserver.java:
##########
@@ -29,57 +34,100 @@
/**
* A minion event observer that can track task progress status in memory.
*/
+@ThreadSafe
public class MinionProgressObserver extends DefaultMinionEventObserver {
private static final Logger LOGGER = LoggerFactory.getLogger(MinionProgressObserver.class);
+ // TODO: make this configurable
+ private static final int DEFAULT_MAX_NUM_STATUS_TO_TRACK = 128;
- private static volatile long _startTs;
- private static volatile Object _lastStatus;
+ private final int _maxNumStatusToTrack;
+ private final Deque<StatusEntry> _lastStatus = new LinkedList<>();
+ private long _startTs;
+
+ public MinionProgressObserver() {
+ this(DEFAULT_MAX_NUM_STATUS_TO_TRACK);
+ }
+
+ public MinionProgressObserver(int maxNumStatusToTrack) {
+ _maxNumStatusToTrack = maxNumStatusToTrack;
+ }
@Override
- public void notifyTaskStart(PinotTaskConfig pinotTaskConfig) {
+ public synchronized void notifyTaskStart(PinotTaskConfig pinotTaskConfig) {
_startTs = System.currentTimeMillis();
+ addStatus(_startTs, "Task started");
super.notifyTaskStart(pinotTaskConfig);
}
- public void notifyProgress(PinotTaskConfig pinotTaskConfig, @Nullable Object progress) {
+ public synchronized void notifyProgress(PinotTaskConfig pinotTaskConfig, @Nullable Object progress) {
if (LOGGER.isDebugEnabled()) {
LOGGER.debug("Update progress: {} for task: {}", progress, pinotTaskConfig.getTaskId());
}
- _lastStatus = progress;
+ addStatus(System.currentTimeMillis(), (progress == null) ? "" : progress.toString());
super.notifyProgress(pinotTaskConfig, progress);
}
@Nullable
- public Object getProgress() {
- return _lastStatus;
+ public synchronized List<StatusEntry> getProgress() {
Review Comment:
ditto: not backward compatible.
--
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 #9449: refine minion worker event observer to track finer grained progress for tasks
Posted by GitBox <gi...@apache.org>.
klsince commented on code in PR #9449:
URL: https://github.com/apache/pinot/pull/9449#discussion_r978160078
##########
pinot-minion/src/main/java/org/apache/pinot/minion/event/MinionProgressObserver.java:
##########
@@ -29,57 +34,100 @@
/**
* A minion event observer that can track task progress status in memory.
*/
+@ThreadSafe
public class MinionProgressObserver extends DefaultMinionEventObserver {
private static final Logger LOGGER = LoggerFactory.getLogger(MinionProgressObserver.class);
+ // TODO: make this configurable
+ private static final int DEFAULT_MAX_NUM_STATUS_TO_TRACK = 128;
- private static volatile long _startTs;
- private static volatile Object _lastStatus;
+ private final int _maxNumStatusToTrack;
+ private final Deque<StatusEntry> _lastStatus = new LinkedList<>();
+ private long _startTs;
+
+ public MinionProgressObserver() {
+ this(DEFAULT_MAX_NUM_STATUS_TO_TRACK);
+ }
+
+ public MinionProgressObserver(int maxNumStatusToTrack) {
+ _maxNumStatusToTrack = maxNumStatusToTrack;
+ }
@Override
- public void notifyTaskStart(PinotTaskConfig pinotTaskConfig) {
+ public synchronized void notifyTaskStart(PinotTaskConfig pinotTaskConfig) {
_startTs = System.currentTimeMillis();
+ addStatus(_startTs, "Task started");
super.notifyTaskStart(pinotTaskConfig);
}
- public void notifyProgress(PinotTaskConfig pinotTaskConfig, @Nullable Object progress) {
+ public synchronized void notifyProgress(PinotTaskConfig pinotTaskConfig, @Nullable Object progress) {
if (LOGGER.isDebugEnabled()) {
LOGGER.debug("Update progress: {} for task: {}", progress, pinotTaskConfig.getTaskId());
}
- _lastStatus = progress;
+ addStatus(System.currentTimeMillis(), (progress == null) ? "" : progress.toString());
Review Comment:
good question. not always but kept things simple for now as all status is String right now. I'll add a method comment that the status.toString() should return sth meaningful, but this is only for `MinionProgressObserver` class, as one can implement a different MinionEventObserver which does not do toString().
--
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 #9449: refine minion worker event observer to track finer grained progress for tasks
Posted by GitBox <gi...@apache.org>.
klsince commented on code in PR #9449:
URL: https://github.com/apache/pinot/pull/9449#discussion_r978155250
##########
pinot-minion/src/main/java/org/apache/pinot/minion/event/MinionProgressObserver.java:
##########
@@ -29,57 +34,100 @@
/**
* A minion event observer that can track task progress status in memory.
*/
+@ThreadSafe
public class MinionProgressObserver extends DefaultMinionEventObserver {
private static final Logger LOGGER = LoggerFactory.getLogger(MinionProgressObserver.class);
+ // TODO: make this configurable
+ private static final int DEFAULT_MAX_NUM_STATUS_TO_TRACK = 128;
- private static volatile long _startTs;
- private static volatile Object _lastStatus;
+ private final int _maxNumStatusToTrack;
+ private final Deque<StatusEntry> _lastStatus = new LinkedList<>();
+ private long _startTs;
+
+ public MinionProgressObserver() {
+ this(DEFAULT_MAX_NUM_STATUS_TO_TRACK);
+ }
+
+ public MinionProgressObserver(int maxNumStatusToTrack) {
+ _maxNumStatusToTrack = maxNumStatusToTrack;
+ }
@Override
- public void notifyTaskStart(PinotTaskConfig pinotTaskConfig) {
+ public synchronized void notifyTaskStart(PinotTaskConfig pinotTaskConfig) {
_startTs = System.currentTimeMillis();
+ addStatus(_startTs, "Task started");
super.notifyTaskStart(pinotTaskConfig);
}
- public void notifyProgress(PinotTaskConfig pinotTaskConfig, @Nullable Object progress) {
+ public synchronized void notifyProgress(PinotTaskConfig pinotTaskConfig, @Nullable Object progress) {
if (LOGGER.isDebugEnabled()) {
LOGGER.debug("Update progress: {} for task: {}", progress, pinotTaskConfig.getTaskId());
}
- _lastStatus = progress;
+ addStatus(System.currentTimeMillis(), (progress == null) ? "" : progress.toString());
super.notifyProgress(pinotTaskConfig, progress);
}
@Nullable
- public Object getProgress() {
- return _lastStatus;
+ public synchronized List<StatusEntry> getProgress() {
+ return new ArrayList<>(_lastStatus);
Review Comment:
as StatusEntry is immutable, this defensive copy should be able to keep internal states safe.
--
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 #9449: refine minion worker event observer to track finer grained progress for tasks
Posted by GitBox <gi...@apache.org>.
klsince commented on code in PR #9449:
URL: https://github.com/apache/pinot/pull/9449#discussion_r978157751
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/mergerollup/MergeRollupTaskExecutor.java:
##########
@@ -55,6 +55,8 @@ public MergeRollupTaskExecutor(MinionConf minionConf) {
protected List<SegmentConversionResult> convert(PinotTaskConfig pinotTaskConfig, List<File> segmentDirs,
File workingDir)
throws Exception {
+ int numInputSegments = segmentDirs.size();
+ _eventObserver.notifyProgress(pinotTaskConfig, "Converting segments: " + numInputSegments);
Review Comment:
same as above, this is a subclass, and the segment name is tracked by caller of convert().
--
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 #9449: refine minion worker event observer to track finer grained progress for tasks
Posted by GitBox <gi...@apache.org>.
klsince commented on code in PR #9449:
URL: https://github.com/apache/pinot/pull/9449#discussion_r978153541
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotHelixTaskResourceManager.java:
##########
@@ -463,7 +463,7 @@ public synchronized Map<String, PinotTaskConfig> getSubtaskConfigs(String taskNa
return taskConfigs;
}
- public synchronized Map<String, String> getSubtaskProgress(String taskName, @Nullable String subtaskNames,
+ public synchronized Map<String, Object> getSubtaskProgress(String taskName, @Nullable String subtaskNames,
Review Comment:
like commented in last msg, this method was added in my last PR yesterday and don't think it's used by any yet, this change makes it more easier to be extended later.
--
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 #9449: refine minion worker event observer to track finer grained progress for tasks
Posted by GitBox <gi...@apache.org>.
zhtaoxiang commented on code in PR #9449:
URL: https://github.com/apache/pinot/pull/9449#discussion_r978132184
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/converttorawindex/ConvertToRowIndexTaskProgressObserverFactory.java:
##########
@@ -0,0 +1,45 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.plugin.minion.tasks.converttorawindex;
+
+import org.apache.pinot.core.common.MinionConstants;
+import org.apache.pinot.minion.event.MinionEventObserver;
+import org.apache.pinot.minion.event.MinionEventObserverFactory;
+import org.apache.pinot.minion.event.MinionProgressObserver;
+import org.apache.pinot.minion.executor.MinionTaskZkMetadataManager;
+import org.apache.pinot.spi.annotations.minion.EventObserverFactory;
+
+
+@EventObserverFactory
+public class ConvertToRowIndexTaskProgressObserverFactory implements MinionEventObserverFactory {
Review Comment:
Since similar code is used multiple times, could we create a common abstract class `MinonTaskProgressFactory` and provide the same `init` and `create?`
--
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 #9449: refine minion worker event observer to track finer grained progress for tasks
Posted by GitBox <gi...@apache.org>.
klsince commented on code in PR #9449:
URL: https://github.com/apache/pinot/pull/9449#discussion_r978154220
##########
pinot-minion/src/main/java/org/apache/pinot/minion/event/MinionProgressObserver.java:
##########
@@ -29,57 +34,100 @@
/**
* A minion event observer that can track task progress status in memory.
*/
+@ThreadSafe
public class MinionProgressObserver extends DefaultMinionEventObserver {
private static final Logger LOGGER = LoggerFactory.getLogger(MinionProgressObserver.class);
+ // TODO: make this configurable
+ private static final int DEFAULT_MAX_NUM_STATUS_TO_TRACK = 128;
- private static volatile long _startTs;
- private static volatile Object _lastStatus;
+ private final int _maxNumStatusToTrack;
+ private final Deque<StatusEntry> _lastStatus = new LinkedList<>();
+ private long _startTs;
+
+ public MinionProgressObserver() {
+ this(DEFAULT_MAX_NUM_STATUS_TO_TRACK);
+ }
+
+ public MinionProgressObserver(int maxNumStatusToTrack) {
+ _maxNumStatusToTrack = maxNumStatusToTrack;
+ }
@Override
- public void notifyTaskStart(PinotTaskConfig pinotTaskConfig) {
+ public synchronized void notifyTaskStart(PinotTaskConfig pinotTaskConfig) {
_startTs = System.currentTimeMillis();
+ addStatus(_startTs, "Task started");
super.notifyTaskStart(pinotTaskConfig);
}
- public void notifyProgress(PinotTaskConfig pinotTaskConfig, @Nullable Object progress) {
+ public synchronized void notifyProgress(PinotTaskConfig pinotTaskConfig, @Nullable Object progress) {
if (LOGGER.isDebugEnabled()) {
LOGGER.debug("Update progress: {} for task: {}", progress, pinotTaskConfig.getTaskId());
}
- _lastStatus = progress;
+ addStatus(System.currentTimeMillis(), (progress == null) ? "" : progress.toString());
super.notifyProgress(pinotTaskConfig, progress);
}
@Nullable
- public Object getProgress() {
- return _lastStatus;
+ public synchronized List<StatusEntry> getProgress() {
+ return new ArrayList<>(_lastStatus);
}
@Override
- public void notifyTaskSuccess(PinotTaskConfig pinotTaskConfig, @Nullable Object executionResult) {
+ public synchronized void notifyTaskSuccess(PinotTaskConfig pinotTaskConfig, @Nullable Object executionResult) {
long endTs = System.currentTimeMillis();
- _lastStatus = "Task succeeded in " + (endTs - _startTs) + "ms";
+ addStatus(endTs, "Task succeeded in " + (endTs - _startTs) + "ms");
super.notifyTaskSuccess(pinotTaskConfig, executionResult);
}
@Override
- public void notifyTaskCancelled(PinotTaskConfig pinotTaskConfig) {
+ public synchronized void notifyTaskCancelled(PinotTaskConfig pinotTaskConfig) {
long endTs = System.currentTimeMillis();
- _lastStatus = "Task got cancelled after " + (endTs - _startTs) + "ms";
+ addStatus(endTs, "Task got cancelled after " + (endTs - _startTs) + "ms");
super.notifyTaskCancelled(pinotTaskConfig);
}
@Override
- public void notifyTaskError(PinotTaskConfig pinotTaskConfig, Exception e) {
+ public synchronized void notifyTaskError(PinotTaskConfig pinotTaskConfig, Exception e) {
long endTs = System.currentTimeMillis();
- _lastStatus = "Task failed in " + (endTs - _startTs) + "ms, with error:\n" + makeStringFromException(e);
+ addStatus(endTs, "Task failed in " + (endTs - _startTs) + "ms with error: " + makeStringFromException(e));
super.notifyTaskError(pinotTaskConfig, e);
}
+ private void addStatus(long ts, String progress) {
Review Comment:
feels not very necessary, as all public methods are sync'ed
--
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 #9449: refine minion worker event observer to track finer grained progress for tasks
Posted by GitBox <gi...@apache.org>.
klsince commented on code in PR #9449:
URL: https://github.com/apache/pinot/pull/9449#discussion_r978156837
##########
pinot-minion/src/main/java/org/apache/pinot/minion/event/MinionProgressObserver.java:
##########
@@ -29,57 +34,100 @@
/**
* A minion event observer that can track task progress status in memory.
*/
+@ThreadSafe
public class MinionProgressObserver extends DefaultMinionEventObserver {
private static final Logger LOGGER = LoggerFactory.getLogger(MinionProgressObserver.class);
+ // TODO: make this configurable
+ private static final int DEFAULT_MAX_NUM_STATUS_TO_TRACK = 128;
- private static volatile long _startTs;
- private static volatile Object _lastStatus;
+ private final int _maxNumStatusToTrack;
+ private final Deque<StatusEntry> _lastStatus = new LinkedList<>();
+ private long _startTs;
+
+ public MinionProgressObserver() {
+ this(DEFAULT_MAX_NUM_STATUS_TO_TRACK);
+ }
+
+ public MinionProgressObserver(int maxNumStatusToTrack) {
+ _maxNumStatusToTrack = maxNumStatusToTrack;
+ }
@Override
- public void notifyTaskStart(PinotTaskConfig pinotTaskConfig) {
+ public synchronized void notifyTaskStart(PinotTaskConfig pinotTaskConfig) {
_startTs = System.currentTimeMillis();
+ addStatus(_startTs, "Task started");
super.notifyTaskStart(pinotTaskConfig);
}
- public void notifyProgress(PinotTaskConfig pinotTaskConfig, @Nullable Object progress) {
+ public synchronized void notifyProgress(PinotTaskConfig pinotTaskConfig, @Nullable Object progress) {
if (LOGGER.isDebugEnabled()) {
LOGGER.debug("Update progress: {} for task: {}", progress, pinotTaskConfig.getTaskId());
}
- _lastStatus = progress;
+ addStatus(System.currentTimeMillis(), (progress == null) ? "" : progress.toString());
super.notifyProgress(pinotTaskConfig, progress);
}
@Nullable
- public Object getProgress() {
- return _lastStatus;
+ public synchronized List<StatusEntry> getProgress() {
Review Comment:
as replied above, I'm trying to refine the methods added in my PR landed yesterday. don't think they would have been used by any.
--
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