Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

handle pending minion tasks properly when getting the task progress status #9911

Merged
merged 2 commits into from
Dec 15, 2022

Conversation

klsince
Copy link
Contributor

@klsince klsince commented Dec 5, 2022

bugfix: when subtasks are waiting in Helix, the get task progress endpoint failed with NPE as those pending subtasks don't have assigned workers yet. This PR handles pending tasks properly.

Validated that the Pinot UI tab Minion Task Manager could show the task progress properly now even with pending subtasks.

@codecov-commenter
Copy link

codecov-commenter commented Dec 5, 2022

Codecov Report

Merging #9911 (ac089c8) into master (f743ff1) will decrease coverage by 37.83%.
The diff coverage is 95.45%.

@@              Coverage Diff              @@
##             master    #9911       +/-   ##
=============================================
- Coverage     70.34%   32.51%   -37.84%     
+ Complexity     5656      191     -5465     
=============================================
  Files          1989     1989               
  Lines        107181   107187        +6     
  Branches      16290    16293        +3     
=============================================
- Hits          75400    34849    -40551     
- Misses        26515    69289    +42774     
+ Partials       5266     3049     -2217     
Flag Coverage Δ
integration1 25.05% <0.00%> (-0.06%) ⬇️
integration2 ?
unittests1 ?
unittests2 13.62% <95.45%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...lix/core/minion/PinotHelixTaskResourceManager.java 43.81% <95.45%> (+3.10%) ⬆️
...in/java/org/apache/pinot/spi/utils/BytesUtils.java 0.00% <0.00%> (-100.00%) ⬇️
...java/org/apache/pinot/spi/trace/BaseRecording.java 0.00% <0.00%> (-100.00%) ⬇️
...java/org/apache/pinot/spi/trace/NoOpRecording.java 0.00% <0.00%> (-100.00%) ⬇️
...ava/org/apache/pinot/spi/config/table/FSTType.java 0.00% <0.00%> (-100.00%) ⬇️
...ava/org/apache/pinot/spi/config/user/RoleType.java 0.00% <0.00%> (-100.00%) ⬇️
...ava/org/apache/pinot/spi/data/MetricFieldSpec.java 0.00% <0.00%> (-100.00%) ⬇️
...ava/org/apache/pinot/spi/stream/StreamMessage.java 0.00% <0.00%> (-100.00%) ⬇️
...java/org/apache/pinot/common/tier/TierFactory.java 0.00% <0.00%> (-100.00%) ⬇️
...a/org/apache/pinot/spi/config/table/TableType.java 0.00% <0.00%> (-100.00%) ⬇️
... and 1218 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@klsince klsince force-pushed the fix_npe_task_progress_api branch from a0446b3 to c85583b Compare December 6, 2022 05:16
Copy link
Contributor

@snleee snleee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to add the testing case to cover this scenario?

@klsince klsince force-pushed the fix_npe_task_progress_api branch from c85583b to ac089c8 Compare December 14, 2022 05:54
@klsince klsince requested a review from snleee December 14, 2022 07:13
Copy link
Contributor

@snleee snleee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you!

@snleee snleee merged commit cf43567 into apache:master Dec 15, 2022
@klsince klsince deleted the fix_npe_task_progress_api branch December 15, 2022 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants