-
Notifications
You must be signed in to change notification settings - Fork 21
STR-1739: ASM Logs in CSM #1078
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
Conversation
|
Commit: 801a821 SP1 Execution Results
|
delbonis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really happy with how this is coming along far. Main comments are on service input handling and logging.
Do you think you should make the change to how ServiceMonitor is defined in this PR or should we do it in a different one?
@delbonis I don't have a strong opinion here. |
|
@delbonis PTAL on service related stuff (I'll address logs tiny bit later). |
|
@evgenyzdanovich I don't see any other open PRs that use the monitor system right now so it's probably fine just to do it here, since this is close to being mergeable and it won't cause much complication. |
delbonis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes to services look good, this is mostly nits, plus the other review comments.
prajwolrg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from the log handling, looks good to me.
It seems you’ve intentionally avoided making major changes to the existing data structures and legacy code in this PR, which is fine. Maybe we should also keep a track of the changes that needs to be done in the upcoming PRs.
59e12cc to
81d4714
Compare
|
mostly finished working on this:
what's left is to resolve proof and checkpoint related stuff. I'll ask for a final review once it's ready. |
81d4714 to
0ba6396
Compare
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #1078 +/- ##
==========================================
+ Coverage 74.97% 75.35% +0.38%
==========================================
Files 499 498 -1
Lines 42123 42406 +283
==========================================
+ Hits 31580 31956 +376
+ Misses 10543 10450 -93
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 31 files with indirect coverage changes 🚀 New features to boost your workflow:
|
bewakes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! It's very good to see the service framework being used/enhanced. :)
There are some nits and suggestions, particularly to add unit tests.
|
Approved the PR just to realize that this needs a rebase. |
…and always allow empty proofs (for now).
058da54 to
febf00a
Compare
|
The dbtools functional tests were consistently failing on this PR but are now passing. The issue was identified in Previously, the bug remained hidden because the CSM was the first consumer of L1 information. This is no longer the case, as the ASM now consumes it first. Since ASM entries should never be removed (as they remain unchanged once committed and processed), the ClientState must be cleaned up and reapplied instead. It was also observed that similar changes are being introduced in this PR, so conflicts may need to be resolved in the PR whichever gets merged later. |
storopoli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
Description
ASMLogs in theCSM.CSMworker as a listener.Note:
parts of the work were done with the use of Claude.
Type of Change
Notes to Reviewers
Checklist
in the body of this PR.
Related Issues