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

(ignore) #11295

Merged
merged 1 commit into from
Dec 13, 2023
Merged

Conversation

msmouse
Copy link
Contributor

@msmouse msmouse commented Dec 12, 2023

Description

Test Plan

Copy link

trunk-io bot commented Dec 12, 2023

⏱️ 41h 32m total CI duration on this PR
Job Cumulative Duration Recent Runs
rust-unit-tests 15h 51m 🟥🟥🟥🟥🟥 (+14 more)
windows-build 8h 57m 🟩🟩🟥🟥🟥 (+31 more)
rust-move-unit-coverage 6h 1m 🟥🟥 (+20 more)
rust-images / rust-all 2h 1m 🟩🟩🟩🟩🟩 (+9 more)
rust-lints 1h 53m 🟩🟩🟩🟥🟥 (+14 more)
check-dynamic-deps 1h 19m 🟩🟩🟩🟩🟩 (+30 more)
run-tests-main-branch 1h 19m 🟩🟩🟩 (+15 more)
check 1h 15m 🟩🟩🟩 (+23 more)
general-lints 47m 🟩🟩🟩🟩🟩 (+14 more)
semgrep/ci 12m 🟩🟩🟩🟩🟩 (+31 more)
replay-mainnet / replay-verify (11) 6m
replay-mainnet / replay-verify (1) 6m
replay-mainnet / replay-verify (12) 6m
replay-mainnet / replay-verify (6) 6m
replay-mainnet / replay-verify (14) 6m
replay-mainnet / replay-verify (4) 6m
replay-mainnet / replay-verify (13) 6m
replay-mainnet / replay-verify (16) 6m
replay-mainnet / replay-verify (10) 6m
replay-mainnet / replay-verify (3) 6m
replay-mainnet / replay-verify (0) 6m
replay-mainnet / replay-verify (7) 6m
replay-mainnet / replay-verify (5) 6m
replay-mainnet / replay-verify (15) 6m
replay-mainnet / replay-verify (2) 6m
replay-mainnet / replay-verify (8) 6m
file_change_determinator 4m 🟩🟩🟩🟩🟩 (+15 more)
file_change_determinator 4m 🟩🟩🟩🟩 (+21 more)
file_change_determinator 3m 🟩🟩🟩🟩🟩 (+9 more)
permission-check 2m 🟩🟩🟩🟩🟩 (+30 more)
permission-check 2m 🟩🟩🟩🟩🟩 (+31 more)
permission-check 1m 🟩🟩🟩🟩 (+22 more)
permission-check 1m 🟩🟩🟩🟩🟩 (+23 more)
determine-docker-build-metadata 42s 🟩🟩🟩🟩🟩 (+9 more)
permission-check 41s 🟩🟩🟩🟩 (+12 more)
determine-test-metadata 8s 🟩
replay-mainnet / replay-verify (9) 1s

settingsfeedbackdocs ⋅ learn more about trunk.io

@msmouse
Copy link
Contributor Author

msmouse commented Dec 12, 2023

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

Copy link

codecov bot commented Dec 12, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (1210-alden-ephemeral-bytes-fee@d2fd5e9). Click here to learn what that means.

❗ Current head 444fa3a differs from pull request most recent head b67a05b. Consider uploading reports for the commit b67a05b to get more accurate results

Additional details and impacted files
@@                        Coverage Diff                        @@
##             1210-alden-ephemeral-bytes-fee   #11295   +/-   ##
=================================================================
  Coverage                                  ?    68.0%           
=================================================================
  Files                                     ?      764           
  Lines                                     ?   177262           
  Branches                                  ?        0           
=================================================================
  Hits                                      ?   120707           
  Misses                                    ?    56555           
  Partials                                  ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@igor-aptos igor-aptos left a comment

Choose a reason for hiding this comment

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

still need to look at the full PR, but this needs to be fixed:

Comment on lines +90 to +95
// FIXME(aldenhu): confirm this is correct
unreachable!("No metadata for delayed field changes")
Copy link
Contributor

Choose a reason for hiding this comment

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

we are iterating over all writes, this is definitely reachable - that's why it was an option?

"InPlaceDelayedFieldChange" means we are doing a write to that state item, it is just that only write is from Aggregator inside it, there are no other updates.

and so the actual io write needs to be charged, it's just that state will stay the same size

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, made the change to let InPlaceDelayedFieldChange carry the metadata as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

folded commit to parent PR.

@msmouse msmouse force-pushed the 1210-alden-ephemeral-bytes-fee branch from ea5a34b to 5d4a279 Compare December 13, 2023 00:46
@msmouse msmouse force-pushed the 1210-alden-ephemeral-bytes-fee branch from 5d4a279 to 0d506a1 Compare December 13, 2023 07:22
@msmouse msmouse force-pushed the 1210-alden-ephemeral-bytes-fee branch from 0d506a1 to ccd4296 Compare December 13, 2023 09:00
@msmouse msmouse force-pushed the 1211-alden-metadata-upgrade branch 3 times, most recently from 8d6ff66 to f07a5b9 Compare December 13, 2023 21:59
So that the bytes deposit can be tracked as soon as possible
@msmouse msmouse force-pushed the 1210-alden-ephemeral-bytes-fee branch from ccd4296 to d2fd5e9 Compare December 13, 2023 22:00
@msmouse msmouse merged commit b67a05b into 1210-alden-ephemeral-bytes-fee Dec 13, 2023
63 of 72 checks passed
@msmouse msmouse deleted the 1211-alden-metadata-upgrade branch December 13, 2023 22:02
@msmouse msmouse changed the title Automatically upgrade StateValueMetadata (ignore) Dec 14, 2023
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.

2 participants