Include the task scheduling errors in schedule response#14754
Merged
swaminathanmanish merged 4 commits intoapache:masterfrom Jan 13, 2025
Merged
Include the task scheduling errors in schedule response#14754swaminathanmanish merged 4 commits intoapache:masterfrom
swaminathanmanish merged 4 commits intoapache:masterfrom
Conversation
shounakmk219
requested review from
KKcorps,
swaminathanmanish and
tibrewalpratik17
shounakmk219
added
backward-incompat
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #14754 +/- ##
============================================
+ Coverage 61.75% 63.84% +2.09%
- Complexity 207 1609 +1402
============================================
Files 2436 2704 +268
Lines 133233 151008 +17775
Branches 20636 23318 +2682
============================================
+ Hits 82274 96412 +14138
- Misses 44911 47380 +2469
- Partials 6048 7216 +1168
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
pinot-controller/src/main/resources/app/pages/TaskQueueTable.tsx
Outdated
Show resolved
Hide resolved
Contributor
jayeshchoudhary
left a comment
jayeshchoudhary
left a comment
There was a problem hiding this comment.
ui changes looks good.
...egration-test-base/src/test/java/org/apache/pinot/integration/tests/MinionTaskTestUtils.java
Outdated
Show resolved
Hide resolved
Comment on lines
+51
to
+54
| assertNotNull(info.getGenerationErrors()); | ||
| assertTrue(info.getGenerationErrors().isEmpty()); | ||
| assertNotNull(info.getSchedulingErrors()); | ||
| assertTrue(info.getSchedulingErrors().isEmpty()); |
Contributor
There was a problem hiding this comment.
Not sure iiuc, but errors can be non-empty too if no task is scheduled due to scheduling / generation errors right?
Collaborator
Author
There was a problem hiding this comment.
The tests are using this util methods right now to assert that tasks are not being scheduled due to no work or tasks already in progress. In that flow we need to make sure that there are no errors as well.
Contributor
There was a problem hiding this comment.
Hmm would suggest to move the last 4 assertions to a separate method maybe -- assertNoError kind of. But can be done in a follow-up PR as well.
Co-authored-by: Pratik Tibrewal <tibrewalpratik17@gmail.com>
tibrewalpratik17
approved these changes
Jan 10, 2025
swaminathanmanish
approved these changes
Jan 13, 2025
Contributor
swaminathanmanish
left a comment
swaminathanmanish
left a comment
There was a problem hiding this comment.
Thanks @shounakmk219 . This has been a pain point for a long time.
We will need similar change for adhoc task as well ?
public void executeAdhocTask(AdhocTaskConfig adhocTaskConfig, @Suspended AsyncResponse asyncResponse,
@Context Request requestContext) {
yashmayya
reviewed
Jan 17, 2025
Contributor
yashmayya
left a comment
yashmayya
left a comment
There was a problem hiding this comment.
This is causing a ton of spammy logs in Quickstart:
2025/01/17 20:04:01.221 ERROR [BootstrapTableTool] [main] Failed to query task endpoint
org.apache.hc.core5.http.HttpException: Unable to get state for task: . Error code 404, Error message: {"code":404,"error":"HTTP 404 Not Found"}
at org.apache.pinot.common.minion.MinionClient.getTaskState(MinionClient.java:110) ~[classes/:?]
at org.apache.pinot.tools.BootstrapTableTool.waitForMinionTaskToFinish(BootstrapTableTool.java:312) [classes/:?]
at org.apache.pinot.tools.BootstrapTableTool.setupOfflineData(BootstrapTableTool.java:218) [classes/:?]
at org.apache.pinot.tools.BootstrapTableTool.bootstrapOfflineTable(BootstrapTableTool.java:203) [classes/:?]
at org.apache.pinot.tools.BootstrapTableTool.execute(BootstrapTableTool.java:112) [classes/:?]
at org.apache.pinot.tools.admin.command.QuickstartRunner.bootstrapTable(QuickstartRunner.java:232) [classes/:?]
at org.apache.pinot.tools.Quickstart.execute(Quickstart.java:86) [classes/:?]
at org.apache.pinot.tools.admin.command.QuickStartCommand.execute(QuickStartCommand.java:169) [classes/:?]
at org.apache.pinot.tools.Command.call(Command.java:33) [classes/:?]
at org.apache.pinot.tools.Command.call(Command.java:29) [classes/:?]
at picocli.CommandLine.executeUserObject(CommandLine.java:2045) [picocli-4.7.6.jar:4.7.6]
at picocli.CommandLine.access$1500(CommandLine.java:148) [picocli-4.7.6.jar:4.7.6]
at picocli.CommandLine$RunLast.executeUserObjectOfLastSubcommandWithSameParent(CommandLine.java:2465) [picocli-4.7.6.jar:4.7.6]
at picocli.CommandLine$RunLast.handle(CommandLine.java:2457) [picocli-4.7.6.jar:4.7.6]
at picocli.CommandLine$RunLast.handle(CommandLine.java:2419) [picocli-4.7.6.jar:4.7.6]
at picocli.CommandLine$AbstractParseResultHandler.execute(CommandLine.java:2277) [picocli-4.7.6.jar:4.7.6]
at picocli.CommandLine$RunLast.execute(CommandLine.java:2421) [picocli-4.7.6.jar:4.7.6]
at picocli.CommandLine.execute(CommandLine.java:2174) [picocli-4.7.6.jar:4.7.6]
at org.apache.pinot.tools.admin.PinotAdministrator.execute(PinotAdministrator.java:174) [classes/:?]
at org.apache.pinot.tools.admin.PinotAdministrator.main(PinotAdministrator.java:210) [classes/:?]
at org.apache.pinot.tools.Quickstart.main(Quickstart.java:151) [classes/:?]
Note that the minion task does eventually succeed and all the tables are bootstrapped correctly.
zeronerdzerogeekzerocool
pushed a commit
to zeronerdzerogeekzerocool/pinot
that referenced
this pull request
Feb 20, 2025
* Include the task scheduling errors in schedule response * fix tests * import fix * fix param name Co-authored-by: Pratik Tibrewal <tibrewalpratik17@gmail.com> --------- Co-authored-by: Pratik Tibrewal <tibrewalpratik17@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The task schedule endpoint on controller does not give the user any visibility into how the scheduling went when there is a
taskType : nullresponseTo relay more info back to the user, this PR enhances the return types of
PinotTaskManagerschedule methods to a wrapper object that contains the generation and scheduling errors along with the names of scheduled tasksIn order to not break the schedule endpoints the error info is returned as separate entry in the response map as below
Earlier response
Successful schedule
Successful schedule with no tasks to schedule or failed schedule
Improved response
Successful schedule without any errors
Successful schedule with no tasks to schedule
Failed schedule with errors during task generation
UI Changes
This PR also makes the UI change for
Schedule NowctaWhen task is scheduled
When there is nothing to schedule
When there are scheduling errors
Backward incompatible changes
The
PinotTaskManagerschedule methods have changed return types from eitherList<String>toTaskSchedulingInfoorMap<String, List<String>>toMap<String, TaskSchedulingInfo>Schedule methods used to return null as scheduled task names list incase of both scheduling failures as well as no task to schedule. Now that behaviour is changed to null in case of scheduling failures and empty list when there is no task to be scheduled.