Skip to content

Conversation

@awdavidson
Copy link
Contributor

What changes were proposed in this pull request?
Ensure all events are marked as understorage, this will result to the LocalStorageMeta being updated when events are processed.

Why are the changes needed?
Currently LocalStorageMeta is only update with metrics from the first event in a given shuffleId and partitionId, the first event updates metrics because there is no entry in partitionsOfStorage and the event get marked as underStorage, however, for future events in the same shuffleId and partitionId selectStorage returns the storage and does not mark the event as underStorage so when updateWriteMetrics is called, event.getUnderStorage() returns null and storage.updateWriteMetrics(metrics); is skipped.

As metrics are not updated correctly, LocalStorage.canWrite will not return the correct result.

Does this PR introduce any user-facing change?
No.

How was this patch tested?
Added unit test which covers multi events for the same shuffleId and partitionId

@awdavidson
Copy link
Contributor Author

@jerqi @zuston as requested

@jerqi jerqi changed the title [#881] fix(followup): Ensure LocalStorageMeta disk size is correctly updated when events are processed for 0.7.0 [#881][0.7] fix(followup): Ensure LocalStorageMeta disk size is correctly updated when events are processed for 0.7.0 May 30, 2023
@jerqi
Copy link
Contributor

jerqi commented Jun 6, 2023

@awdavidson Could you fix this issue?

@awdavidson
Copy link
Contributor Author

@awdavidson Could you fix this issue?

@jerqi please can you approve the workflow?

@jerqi
Copy link
Contributor

jerqi commented Jun 6, 2023

@awdavidson What's your company? Do you have Wechat account? We have aWechat group. If you want to join the Wechat group, you can scan the QR code.
21eca427bcf8f91c98456e6a58d727c4

@codecov-commenter
Copy link

codecov-commenter commented Jun 6, 2023

Codecov Report

Merging #914 (e6c5b3d) into branch-0.7 (5017585) will increase coverage by 0.01%.
The diff coverage is 66.66%.

@@               Coverage Diff                @@
##             branch-0.7     #914      +/-   ##
================================================
+ Coverage         60.75%   60.76%   +0.01%     
- Complexity         1795     1796       +1     
================================================
  Files               214      214              
  Lines             12383    12385       +2     
  Branches           1047     1048       +1     
================================================
+ Hits               7523     7526       +3     
+ Misses             4455     4454       -1     
  Partials            405      405              
Impacted Files Coverage Δ
...he/uniffle/server/storage/LocalStorageManager.java 86.18% <66.66%> (+0.15%) ⬆️

... and 2 files with indirect coverage changes

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

Copy link
Contributor

@jerqi jerqi left a comment

Choose a reason for hiding this comment

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

Thanks @awdavidson , merged to branch 0.7.

@jerqi jerqi merged commit 1339121 into apache:branch-0.7 Jun 6, 2023
@awdavidson
Copy link
Contributor Author

@awdavidson What's your company? Do you have Wechat account? We have aWechat group. If you want to join the Wechat group, you can scan the QR code.
21eca427bcf8f91c98456e6a58d727c4

I’ve been investigating in my personal time currently not related to employer. I do not have a WeChat, I’ll consider getting one.

Thanks for merge 🙂

@jerqi
Copy link
Contributor

jerqi commented Jun 6, 2023

I’ve been investigating in my personal time currently not related to employer.

@awdavidson Thanks for your investigation. If you don't have Wechat, we usually use Chinese to communicate in the Wechat Group. we can also use English to communicate in the slack.
https://join.slack.com/t/the-asf/shared_invite/zt-1fm9561yr-uzTpjqg3jf5nxSJV5AE3KQ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants