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

Update minion task metadata ZNode path #8959

Merged
merged 7 commits into from
Jun 28, 2022

Conversation

zhtaoxiang
Copy link
Contributor

@zhtaoxiang zhtaoxiang commented Jun 23, 2022

What does this PR do?

This PR updates minion task metadata ZNode path from /MINION_TASK_METADATA/${taskType}/${tableNameWithType} to /MINION_TASK_METADATA/${tableNameWithType}/${taskType}.

The logic is backward compatible: if there exists minion task metadata in ZNode path /MINION_TASK_METADATA/${tableNameWithType}/${taskType}, it keeps using it; otherwise, it uses the new ZNode path.

How was the code tested?

  1. It was tested by unit tests
  2. It was tested in a local Pinot cluster: in the case that (1) there exists minion task metadata in ZNode path /MINION_TASK_METADATA/${taskType}/${tableNameWithType}, and (2) there does not exists minion task metadata.

@codecov-commenter
Copy link

codecov-commenter commented Jun 23, 2022

Codecov Report

Merging #8959 (68d681f) into master (7c15bc3) will decrease coverage by 43.03%.
The diff coverage is 33.33%.

❗ Current head 68d681f differs from pull request most recent head e800c2a. Consider uploading reports for the commit e800c2a to get more accurate results

@@              Coverage Diff              @@
##             master    #8959       +/-   ##
=============================================
- Coverage     69.70%   26.66%   -43.04%     
+ Complexity     4709        1     -4708     
=============================================
  Files          1817     1806       -11     
  Lines         94791    94534      -257     
  Branches      14178    14149       -29     
=============================================
- Hits          66073    25207    -40866     
- Misses        24090    66926    +42836     
+ Partials       4628     2401     -2227     
Flag Coverage Δ
integration1 26.66% <33.33%> (+0.12%) ⬆️
integration2 ?
unittests1 ?
unittests2 ?

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

Impacted Files Coverage Δ
...g/apache/pinot/common/minion/BaseTaskMetadata.java 20.00% <ø> (-40.00%) ⬇️
...e/pinot/common/minion/MergeRollupTaskMetadata.java 94.73% <ø> (ø)
.../minion/RealtimeToOfflineSegmentsTaskMetadata.java 100.00% <ø> (ø)
...he/pinot/common/utils/helix/FakePropertyStore.java 0.00% <0.00%> (ø)
...troller/helix/core/minion/ClusterInfoAccessor.java 79.31% <ø> (ø)
.../pinot/core/common/datablock/DataBlockBuilder.java 18.48% <0.00%> (-50.03%) ⬇️
...t/minion/executor/MinionTaskZkMetadataManager.java 100.00% <ø> (ø)
...on/tasks/mergerollup/MergeRollupTaskGenerator.java 76.97% <ø> (-7.89%) ⬇️
...egments/RealtimeToOfflineSegmentsTaskExecutor.java 92.64% <ø> (-1.48%) ⬇️
...gments/RealtimeToOfflineSegmentsTaskGenerator.java 69.11% <ø> (-25.74%) ⬇️
... and 1332 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c15bc3...e800c2a. Read the comment docs.

Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

Ideally, we want to automatically move the ZNRecord from the old path to the new path. When reading the ZK metadata, if we find the old path exist:

  1. If the new one also exist, remove the old one
  2. If the new one does not exist, copy the old record to the new path, then remove the old one

@zhtaoxiang
Copy link
Contributor Author

zhtaoxiang commented Jun 24, 2022

Ideally, we want to automatically move the ZNRecord from the old path to the new path. When reading the ZK metadata, if we find the old path exist:

  1. If the new one also exist, remove the old one
  2. If the new one does not exist, copy the old record to the new path, then remove the old one

We can do 1 safely.
However, 2 is not easy, since the copy-remove operation needs to be atomic. Otherwise, we cannot guarantee that the old and the new ZNodes hold the same data when we delete the old one, so we will never know when it's safe to remove the old ZKNode.

I'd prefer we keep the current logic and discuss how to implement the proposal correctly before going that direction.

@npawar npawar added release-notes Referenced by PRs that need attention when compiling the next release notes enhancement labels Jun 27, 2022
Copy link
Contributor

@npawar npawar left a comment

Choose a reason for hiding this comment

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

minor comment. lgtm otherwise

@zhtaoxiang zhtaoxiang force-pushed the update-minion-task-metadata branch from 664cc08 to e800c2a Compare June 28, 2022 20:37
@walterddr walterddr merged commit 72c2e77 into apache:master Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement release-notes Referenced by PRs that need attention when compiling the next release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants