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

feat: Add ENTITY_COUNTS state in EntityIdService #17421

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Neeharika-Sompalli
Copy link
Member

@Neeharika-Sompalli Neeharika-Sompalli commented Jan 16, 2025

Related to #17316
Fixes #17427

  • Adds ENTITY_COUNTS state in EntityIdService in 0.59
  • Adds post-upgrade step to count all the KV state sizes and update the ENTITY_COUNTS
  • The KVState.size will be deprecated and should not be used any more. Future PRs will remove all the references of it and use ENTITY_COUNTS state instead

Testing:

  • Unit tests
  • Loading state and validating the entity counts map is updated correctly

Signed-off-by: Neeharika-Sompalli <[email protected]>
Signed-off-by: Neeharika-Sompalli <[email protected]>
Copy link

codacy-production bot commented Jan 16, 2025

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
+0.03% (target: -1.00%) 100.00%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (0129e3a) 96992 70493 72.68%
Head commit (fea6a66) 97106 (+114) 70607 (+114) 72.71% (+0.03%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#17421) 134 134 100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

Copy link

codecov bot commented Jan 16, 2025

Codecov Report

Attention: Patch coverage is 99.25373% with 1 line in your changes missing coverage. Please review.

Project coverage is 68.78%. Comparing base (0129e3a) to head (fea6a66).

Files with missing lines Patch % Lines
...com/hedera/node/app/ids/WritableEntityIdStore.java 95.23% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main   #17421      +/-   ##
============================================
+ Coverage     68.74%   68.78%   +0.03%     
- Complexity    22590    22626      +36     
============================================
  Files          2610     2612       +2     
  Lines         97209    97323     +114     
  Branches      10129    10131       +2     
============================================
+ Hits          66828    66943     +115     
  Misses        26565    26565              
+ Partials       3816     3815       -1     
Files with missing lines Coverage Δ
...com/hedera/node/app/spi/validation/EntityType.java 100.00% <100.00%> (ø)
...om/hedera/node/app/blocks/impl/BlockImplUtils.java 80.16% <100.00%> (+0.16%) ⬆️
...e/app/blocks/impl/BoundaryStateChangeListener.java 63.33% <100.00%> (+0.83%) ⬆️
.../java/com/hedera/node/app/ids/EntityIdService.java 100.00% <100.00%> (ø)
...om/hedera/node/app/ids/EntityNumGeneratorImpl.java 100.00% <100.00%> (ø)
...hedera/node/app/ids/ReadableEntityIdStoreImpl.java 100.00% <100.00%> (ø)
...dera/node/app/ids/schemas/V0590EntityIdSchema.java 100.00% <100.00%> (ø)
.../com/hedera/node/app/info/DiskStartupNetworks.java 83.54% <100.00%> (ø)
...era/node/app/roster/schemas/V0540RosterSchema.java 100.00% <100.00%> (ø)
...hedera/node/app/services/MigrationContextImpl.java 47.05% <100.00%> (ø)
... and 15 more

... and 11 files with indirect coverage changes

Impacted file tree graph

@Neeharika-Sompalli Neeharika-Sompalli added this to the v0.59 milestone Jan 16, 2025
@Neeharika-Sompalli Neeharika-Sompalli self-assigned this Jan 16, 2025
Signed-off-by: Neeharika-Sompalli <[email protected]>
Signed-off-by: Neeharika-Sompalli <[email protected]>
Signed-off-by: Neeharika-Sompalli <[email protected]>
@Neeharika-Sompalli Neeharika-Sompalli marked this pull request as ready for review January 17, 2025 02:07
Copy link
Collaborator

@tinker-michaelj tinker-michaelj left a comment

Choose a reason for hiding this comment

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

LGTM, very nice work @Neeharika-Sompalli !

Signed-off-by: Neeharika-Sompalli <[email protected]>
Copy link
Collaborator

@tinker-michaelj tinker-michaelj left a comment

Choose a reason for hiding this comment

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

LGTM, tyvm @Neeharika-Sompalli !

/**
* The number of contract bytecodes in the network.
*/
uint64 num_contract_bytecodes = 13;
Copy link
Member

Choose a reason for hiding this comment

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

Are all these counters needed? I assume counter updates are not free from performance perspective, so if a counter is actually not needed, there is no need to track it...

SCHEDULE,
CONTRACT_BYTECODE,
CONTRACT_STORAGE,
STAKING_INFO
Copy link
Member

Choose a reason for hiding this comment

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

I don't know where this enum is used. Are enum ordinals persisted anywhere? A change like this will affect the ordinals

import edu.umd.cs.findbugs.annotations.NonNull;
import java.util.Set;

public class V0590EntityIdSchema extends Schema {
Copy link
Member

Choose a reason for hiding this comment

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

Should it be named V059EntityCountersSchema instead of V059EntityIdSchema? There is alread "entity ID schema" in 0.49

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.

Add a new singleton ENTITY_COUNTS and add a post upgrade step to update this state
3 participants