Skip to content

Change test to tease NPEs during DefaultBlockchain.<init>() caused by initilization order#7607

Merged
ahamlat merged 2 commits intobesu-eth:mainfrom
lu-pinto:add-test-for-NPEs-due-to-init-order
Sep 12, 2024
Merged

Change test to tease NPEs during DefaultBlockchain.<init>() caused by initilization order#7607
ahamlat merged 2 commits intobesu-eth:mainfrom
lu-pinto:add-test-for-NPEs-due-to-init-order

Conversation

@lu-pinto
Copy link
Copy Markdown
Contributor

@lu-pinto lu-pinto commented Sep 11, 2024 •
edited
Loading

PR description

This complements PR #7601 to make sure possible NPEs are checked during unit tests around DefaultBlockchain when the right order of initialization is not followed.

Fixed Issue(s)

Thanks for sending a pull request! Have you done the following?

  • Checked out our contribution guidelines?
  • Considered documentation and added the doc-change-required label to this PR if updates are required.
  • Considered the changelog and included an update if required.
  • For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

  • unit tests: ./gradlew build
  • acceptance tests: ./gradlew acceptanceTest
  • integration tests: ./gradlew integrationTest
  • reference tests: ./gradlew ethereum:referenceTests:referenceTests

@lu-pinto lu-pinto force-pushed the add-test-for-NPEs-due-to-init-order branch from ddd5b0e to 4d53a01 Compare September 11, 2024 16:50
@lu-pinto lu-pinto marked this pull request as ready for review September 11, 2024 16:50
@lu-pinto lu-pinto self-assigned this Sep 11, 2024
@lu-pinto lu-pinto force-pushed the add-test-for-NPEs-due-to-init-order branch from 4d53a01 to 3e6da91 Compare September 11, 2024 18:22
garyschulte
garyschulte approved these changes Sep 11, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

Non-blocking style comment. Otherwise LGTM

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/DefaultBlockchain.java
Comment on lines +150 to +164
gasUsedCounter =
metricsSystem.createCounter(
BesuMetricCategory.BLOCKCHAIN, "chain_head_gas_used_counter", "Counter for Gas used");

numberOfTransactionsCounter =
metricsSystem.createCounter(
BesuMetricCategory.BLOCKCHAIN,
"chain_head_transaction_count_counter",
"Counter for the number of transactions");
Copy link
Copy Markdown
Contributor

@garyschulte garyschulte Sep 11, 2024

Choose a reason for hiding this comment

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

Seem to me it would be tidier to have these in the createGauges() (perhaps rename createMetrics()) method or similar, and declutter the constructor.

Copy link
Copy Markdown
Contributor

@ahamlat ahamlat Sep 12, 2024

Choose a reason for hiding this comment

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

Or have another method, createCounters if we want to separate Counters from Gauges ;)

Copy link
Copy Markdown
Contributor Author

@lu-pinto lu-pinto Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

sure, I think I didn't put those in a method because I would have to drop the final qualifier from the fields.
But I'll go with @ahamlat's suggestion and create another method for counters

ahamlat
ahamlat approved these changes Sep 12, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@ahamlat ahamlat left a comment

Choose a reason for hiding this comment

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

A small comment related to using the ArrayDeque instead of ArrayList, but it is not blocking.

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/DefaultBlockchain.java
Comment on lines +150 to +164
gasUsedCounter =
metricsSystem.createCounter(
BesuMetricCategory.BLOCKCHAIN, "chain_head_gas_used_counter", "Counter for Gas used");

numberOfTransactionsCounter =
metricsSystem.createCounter(
BesuMetricCategory.BLOCKCHAIN,
"chain_head_transaction_count_counter",
"Counter for the number of transactions");
Copy link
Copy Markdown
Contributor

@ahamlat ahamlat Sep 12, 2024

Choose a reason for hiding this comment

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

Or have another method, createCounters if we want to separate Counters from Gauges ;)

@lu-pinto
Change test to tease NPEs during DefaultBlockchain.<init>() caused by…
4f301ee 
… initilization order

Signed-off-by: Luis Pinto <luis.pinto@consensys.net>
@lu-pinto
fixup! Change test to tease NPEs during DefaultBlockchain.<init>() ca…
ab90a1b 
…used by initilization order

 move counters into their own method

Signed-off-by: Luis Pinto <luis.pinto@consensys.net>
@lu-pinto lu-pinto force-pushed the add-test-for-NPEs-due-to-init-order branch from 12c254d to ab90a1b Compare September 12, 2024 11:14
@ahamlat ahamlat merged commit e0cd508 into besu-eth:main Sep 12, 2024
@lu-pinto lu-pinto deleted the add-test-for-NPEs-due-to-init-order branch September 12, 2024 11:57
@ahamlat ahamlat mentioned this pull request Sep 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@garyschulte garyschulte garyschulte approved these changes

@ahamlat ahamlat ahamlat approved these changes

Assignees

@lu-pinto lu-pinto

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lu-pinto @garyschulte @ahamlat