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

@peter/detailed metrics #256

Merged
merged 14 commits into from
Nov 13, 2024
Merged

@peter/detailed metrics #256

merged 14 commits into from
Nov 13, 2024

Conversation

0g-peterzhb
Copy link
Contributor

@0g-peterzhb 0g-peterzhb commented Nov 1, 2024

This change is Reviewable

@0g-peterzhb 0g-peterzhb marked this pull request as ready for review November 13, 2024 07:10
@0g-peterzhb 0g-peterzhb force-pushed the @peter/detailed-metrics branch 2 times, most recently from 94a9591 to 0f36f3e Compare November 13, 2024 07:13
Copy link
Contributor

@boqiu boqiu left a comment

Choose a reason for hiding this comment

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

Reviewed 15 of 15 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @0g-peterzhb)

Copy link
Contributor

@peilun-conflux peilun-conflux left a comment

Choose a reason for hiding this comment

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

Reviewed 15 of 15 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @0g-peterzhb)


node/storage-async/src/lib.rs line 160 at r1 (raw file):

            let mut frames = backtrace.frames().to_vec();
            if frames.len() > 3 {
                frames.drain(0..); //Remove the first 3 unnecessary frames to simplify// backtrace

This will remove all the frames?

Copy link
Contributor

@peilun-conflux peilun-conflux left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @0g-peterzhb)


node/storage-async/src/lib.rs line 147 at r1 (raw file):

            move || {
                // FIXME(zz): Not all functions need `write`. Refactor store usage.
                let res = f(&*store);

We should print the backtrace here? rx.await will not return error if the receiver is dropped.

Copy link
Contributor Author

@0g-peterzhb 0g-peterzhb left a comment

Choose a reason for hiding this comment

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

Reviewable status: 14 of 15 files reviewed, 2 unresolved discussions (waiting on @boqiu and @peilun-conflux)


node/storage-async/src/lib.rs line 147 at r1 (raw file):

Previously, peilun-conflux wrote…

We should print the backtrace here? tx.await will not return error if the receiver is dropped.

Done.


node/storage-async/src/lib.rs line 160 at r1 (raw file):

Previously, peilun-conflux wrote…

This will remove all the frames?

Done.

Copy link
Contributor

@peilun-conflux peilun-conflux left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @0g-peterzhb)

@0g-peterzhb 0g-peterzhb merged commit f4d5228 into main Nov 13, 2024
5 of 7 checks passed
@0g-peterzhb 0g-peterzhb deleted the @peter/detailed-metrics branch November 13, 2024 09:07
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