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

Create State implementation with registering all of the specific ReadableKVStates #9700

Merged
merged 13 commits into from
Nov 15, 2024

Conversation

bilyana-gospodinova
Copy link
Contributor

@bilyana-gospodinova bilyana-gospodinova commented Nov 4, 2024

Description:
This PR adds a MirrorNodeState class that implements the State interface. The class registers all of needed readable and writable states. Each service (TokenService, ContractService or FileService) needs a different set of states in order to function properly. The readable states are initialised only once and are singleton instances and will be used by all of the threads. They are the implementations of all of the ReadableKVStates. For the writable states we need only 1 facade implementation since no actual changes will be persisted from the memory. The writable state uses a readable state for the read operations and the modifications are kept in memory and discarded at the end of the transaction execution.

Fixes #9259

@bilyana-gospodinova bilyana-gospodinova added enhancement Type: New feature web3 Area: Web3 API labels Nov 4, 2024
@bilyana-gospodinova bilyana-gospodinova self-assigned this Nov 4, 2024
@bilyana-gospodinova bilyana-gospodinova changed the title 09259 state implementation Create State implementation with registering all of the specific ReadableKVStates Nov 4, 2024
Copy link

codecov bot commented Nov 4, 2024

Codecov Report

Attention: Patch coverage is 95.53571% with 10 lines in your changes missing coverage. Please review.

Project coverage is 92.25%. Comparing base (e179868) to head (b724cbb).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
.../com/hedera/mirror/web3/state/MirrorNodeState.java 90.99% 0 Missing and 10 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #9700      +/-   ##
============================================
+ Coverage     92.23%   92.25%   +0.02%     
- Complexity     7628     7739     +111     
============================================
  Files           937      945       +8     
  Lines         32119    32343     +224     
  Branches       4071     4105      +34     
============================================
+ Hits          29624    29838     +214     
  Misses         1542     1542              
- Partials        953      963      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bilyana-gospodinova bilyana-gospodinova force-pushed the 09259-state-implementation branch from b20e20b to ce8ef43 Compare November 4, 2024 12:40
@bilyana-gospodinova bilyana-gospodinova force-pushed the 09259-state-implementation branch from ce8ef43 to 7b1d58d Compare November 5, 2024 14:05
@bilyana-gospodinova bilyana-gospodinova marked this pull request as ready for review November 5, 2024 14:52
@bilyana-gospodinova bilyana-gospodinova requested a review from a team as a code owner November 5, 2024 14:52
@steven-sheehy steven-sheehy added this to the 0.118.0 milestone Nov 5, 2024
@bilyana-gospodinova bilyana-gospodinova marked this pull request as draft November 6, 2024 12:50
@steven-sheehy steven-sheehy modified the milestones: 0.118.0, 0.119.0 Nov 11, 2024
Signed-off-by: Bilyana Gospodinova <bilyana.gospodinova14@gmail.com>
Signed-off-by: Bilyana Gospodinova <bilyana.gospodinova14@gmail.com>
Signed-off-by: Bilyana Gospodinova <bilyana.gospodinova14@gmail.com>
Signed-off-by: Bilyana Gospodinova <bilyana.gospodinova14@gmail.com>
Signed-off-by: Bilyana Gospodinova <bilyana.gospodinova14@gmail.com>
Signed-off-by: Bilyana Gospodinova <bilyana.gospodinova14@gmail.com>
Signed-off-by: Bilyana Gospodinova <bilyana.gospodinova14@gmail.com>
Signed-off-by: Bilyana Gospodinova <bilyana.gospodinova14@gmail.com>
Signed-off-by: Bilyana Gospodinova <bilyana.gospodinova14@gmail.com>
Signed-off-by: Bilyana Gospodinova <bilyana.gospodinova14@gmail.com>
Signed-off-by: Bilyana Gospodinova <bilyana.gospodinova14@gmail.com>
Copy link
Contributor

@IvanKavaldzhiev IvanKavaldzhiev left a comment

Choose a reason for hiding this comment

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

At first glance the PR looks reasonable and have all of the needed state core components. The state should be utilized via integration tests (added in separate PR/PRs) to validate the correctness and consistency of the state. A lot of pitfalls or little issues can be missed without running actual transactions against this state. So I think this PR can be merged in this form and if it needs some touch ups - they can be made in separate PRs.

Signed-off-by: Bilyana Gospodinova <bilyana.gospodinova14@gmail.com>
Signed-off-by: Bilyana Gospodinova <bilyana.gospodinova14@gmail.com>
@bilyana-gospodinova bilyana-gospodinova marked this pull request as ready for review November 12, 2024 11:55
@kselveliev kselveliev self-requested a review November 13, 2024 14:44
Copy link
Member

@steven-sheehy steven-sheehy left a comment

Choose a reason for hiding this comment

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

Mostly just questions

Comment on lines +28 to +29
* {@link Map}. Test code has the option of creating an instance disregarding the backing map, or by
* supplying the backing map to use. This latter option is useful if you want to use Mockito to spy
Copy link
Member

Choose a reason for hiding this comment

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

How would one disregard the backing map? It's required and will throw a NPE if not supplied

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that code was updated but the comment was not. I will update it, thanks.

data.put(stateName, withAnyRegisteredListeners(serviceName, stateName, ref));
}
}
return new MapWritableStates(data, () -> readableStates.remove(serviceName));
Copy link
Member

Choose a reason for hiding this comment

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

Will this cause a commit to flush the readable state and cause it to get re-created for every request?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it will. I tested it without the runnable and it seems to work fine for 1 request. This was added in the example code probably for a reason. I will remove it for now and we will investigate the behaviour on parallel requests later.

@steven-sheehy steven-sheehy merged commit 8a78609 into main Nov 15, 2024
34 checks passed
@steven-sheehy steven-sheehy deleted the 09259-state-implementation branch November 15, 2024 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Type: New feature web3 Area: Web3 API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create State implementation with registering all of the specific ReadableKVStates
4 participants