Skip to content

feat: rawdb.InspectDatabaseOption #111

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Feb 7, 2025
Merged

feat: rawdb.InspectDatabaseOption #111

merged 7 commits into from
Feb 7, 2025

Conversation

@ARR4N
Copy link
Collaborator

@ARR4N ARR4N commented Feb 3, 2025 •
edited
Loading

Why this should be merged

Allows for ava-labs/coreth equivalent modifications of rawdb.InspectDatabase() through external logic injection.

How this works

Variadic options to:

  1. Record a database statistic;
  2. Mark a database statistic as metadata;
  3. Filter statistics for printing.

How this was tested

Testable example acting as a unit test.

@ARR4N ARR4N marked this pull request as ready for review February 3, 2025 11:54
@ARR4N ARR4N requested review from a team, ceyonur, darioush and qdm12 and removed request for a team February 3, 2025 11:55
@qdm12
Copy link

qdm12 commented Feb 3, 2025

Do we need tests of this?

I'm happy to write one and PR it upstream too, let me know 😉

qdm12
qdm12 reviewed Feb 3, 2025
View reviewed changes
darioush
darioush reviewed Feb 3, 2025
View reviewed changes
Copy link

@darioush darioush left a comment

Choose a reason for hiding this comment

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

Let's add a test for the new behavior and remove WithDatabaseStatRecorder if it is unused. Then LGTM

@ARR4N @qdm12
Apply suggestions from code review
7429998 
Co-authored-by: Quentin McGaw <[email protected]>
Signed-off-by: Arran Schlosberg <[email protected]>
@qdm12
Copy link

qdm12 commented Feb 3, 2025

and remove WithDatabaseStatRecorder if it is unused

WithDatabaseStatRecorder will be used by coreth 😉 I'll update the coreth branches soon.

@darioush
Copy link

darioush commented Feb 3, 2025

If it's used then let's add the UT

@ARR4N ARR4N requested a review from qdm12 February 6, 2025 11:07
@qdm12 qdm12 mentioned this pull request Feb 7, 2025
Merged
qdm12 and others added 2 commits February 7, 2025 16:06
@qdm12
chore(core/rawdb): ExampleInspectDatabase test (#119)
600f97e 
## Why this should be merged

Add an example test for `InspectDatabase` with options.

## How this works

See example code
@ARR4N
refactor: remove -1 from stubIterator literal
3deaa98
qdm12
qdm12 approved these changes Feb 7, 2025
View reviewed changes
Copy link

@qdm12 qdm12 left a comment

Choose a reason for hiding this comment

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

Pre-approving to get this merged 🛥️ !

ARR4N and others added 2 commits February 7, 2025 17:16
@ARR4N ARR4N enabled auto-merge (squash) February 7, 2025 17:27
@ARR4N ARR4N merged commit c74b645 into main Feb 7, 2025
4 checks passed
@ARR4N ARR4N deleted the arr4n/rawdb-inspect-stats branch February 7, 2025 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ceyonur ceyonur Awaiting requested review from ceyonur ceyonur was automatically assigned from ava-labs/evm-team

2 more reviewers

@darioush darioush darioush left review comments

@qdm12 qdm12 qdm12 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ARR4N @qdm12 @darioush