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.

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

refactor: 13084 Split Service into Service and RpcService and migrated Service to platform-sdk #13331

Merged
merged 4 commits into from
Jun 5, 2024
Merged

refactor: 13084 Split Service into Service and RpcService and migrated Service to platform-sdk #13331

merged 4 commits into from
Jun 5, 2024

Conversation

imalygin
Copy link
Member

@imalygin imalygin commented May 15, 2024 •
edited

Description:

Highlights of the PR:

  • Split Service interface into two interfaces - RpcService and Service. As the name suggests, RpcService comes with RPC definition.
  • Migrated Service interface and the classes it depends on to swirlds-state-api

Related issue(s):

Fixes #13084

Notes for reviewer:

This refactoring is required to make #11771 possible.

@imalygin imalygin requested review from a team and tinker-michaelj as code owners May 15, 2024 20:39
@imalygin imalygin force-pushed the 13084-app-state branch 2 times, most recently from ff08245 to fca5f3f Compare May 20, 2024 14:56
@imalygin imalygin changed the title Split Service into Service and RpcService and migrated Service to platform-sdk chore: 13084 Split Service into Service and RpcService and migrated Service to platform-sdk May 20, 2024
@imalygin imalygin force-pushed the 13084-app-state branch 3 times, most recently from 6621d15 to 2798835 Compare May 20, 2024 20:44
@github-actions GitHub Actions
Copy link

github-actions bot commented May 20, 2024 •
edited

Node: HAPI Test (Restart) Results

3 tests   3 ✅  6m 2s ⏱️
3 suites  0 💤
3 files    0 ❌

Results for commit 5e4df35.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented May 20, 2024 •
edited

Node: HAPI Test (Node Death Reconnect) Results

3 tests   3 ✅  7m 1s ⏱️
3 suites  0 💤
3 files    0 ❌

Results for commit 5e4df35.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented May 20, 2024 •
edited

Node: HAPI Test (Token) Results

 20 files   20 suites   5m 52s ⏱️
257 tests 257 ✅ 0 💤 0 ❌
260 runs  260 ✅ 0 💤 0 ❌

Results for commit 5e4df35.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented May 20, 2024 •
edited

Node: HAPI Test (Crypto) Results

 23 files   23 suites   12m 6s ⏱️
350 tests 350 ✅ 0 💤 0 ❌
356 runs  356 ✅ 0 💤 0 ❌

Results for commit 5e4df35.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented May 20, 2024 •
edited

Node: HAPI Test (Misc) Results

 51 files   51 suites   22m 35s ⏱️
357 tests 357 ✅ 0 💤 0 ❌
374 runs  374 ✅ 0 💤 0 ❌

Results for commit 5e4df35.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented May 20, 2024 •
edited

Node: HAPI Test (Time Consuming) Results

19 tests   19 ✅  21m 35s ⏱️
 4 suites   0 💤
 4 files     0 ❌

Results for commit 5e4df35.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented May 20, 2024 •
edited

Node: Unit Test Results

  2 318 files  ±0    2 318 suites  ±0   2h 41m 20s ⏱️ - 54m 38s
112 665 tests ±0  112 596 ✅ ±0  69 💤 ±0  0 ❌ ±0 
121 176 runs  ±0  121 107 ✅ ±0  69 💤 ±0  0 ❌ ±0 

Results for commit 5e4df35. ± Comparison against base commit 3bfe8f9.

This pull request removes 3974 and adds 3731 tests. Note that renamed tests count towards both. 

  
             IssuerDN: CN=s-aaaa
            SubjectDN: CN=s-aaaa
           Final Date: Fri Jan 01 00:00:00 UTC 2100
           Public Key: RSA Public Key [2e:28:bc:1e:d3:83:25:92:8e:cb:98:b1:b6:84:06:9c:d5:d8:14:d5],[56:66:d1:a4]
           Start Date: Sat Jan 01 00:00:00 UTC 2000
         SerialNumber: 12482092706667292405
        modulus: c1a0ff5d2372b53d12d12bb87dd03f5…
   Address[id=0,nickname=4e6ZUpwJ,selfName=aaaa,weight=1000,hostnameInternal=127.0.0.1,portInternalIpv4=21479,hostnameExternal=195.0.90.55,portExternalIpv4=21479,sigPublicKey=<null>,agreePublicKey=<null>,sigCert=com.swirlds.platform.crypto.SerializableX509Certificate@71673ad6,agreeCert=com.swirlds.platform.crypto.SerializableX509Certificate@4b5d8cab,memo=EfhG0hMF],
…

com.hedera.node.app.grpc.impl.netty.GrpcServiceBuilderTest ‑ [4] 

com.hedera.node.app.grpc.impl.netty.GrpcServiceBuilderTest ‑ [6] 

com.hedera.node.app.grpc.impl.netty.GrpcServiceBuilderTest ‑ [7]   
  
com.hedera.node.app.service.mono.state.codec.VirtualKeySerdesAdapterTest ‑ [10] com.hedera.node.app.service.mono.state.codec.VirtualBlobKey@3e5773a8
com.hedera.node.app.service.mono.state.codec.VirtualKeySerdesAdapterTest ‑ [11] com.hedera.node.app.service.mono.state.codec.VirtualBlobKey@3e85bded
com.hedera.node.app.service.mono.state.codec.VirtualKeySerdesAdapterTest ‑ [12] com.hedera.node.app.service.mono.state.codec.VirtualBlobKey@c193973b
com.hedera.node.app.service.mono.state.codec.VirtualKeySerdesAdapterTest ‑ [13] com.hedera.node.app.service.mono.state.codec.VirtualBlobKey@c7739e65
com.hedera.node.app.service.mono.state.codec.VirtualKeySerdesAdapterTest ‑ [14] com.hedera.node.app.service.mono.state.codec.VirtualBlobKey@c75441e0
com.hedera.node.app.service.mono.state.codec.VirtualKeySerdesAdapterTest ‑ [15] com.hedera.node.app.service.mono.state.codec.VirtualBlobKey@6591d09
com.hedera.node.app.service.mono.state.codec.VirtualKeySerdesAdapterTest ‑ [16] com.hedera.node.app.service.mono.state.codec.VirtualBlobKey@bf851542
…

♻️ This comment has been updated with latest results.

mhess-swl
mhess-swl reviewed May 21, 2024
View reviewed changes
hedera-node/hedera-app/src/itest/java/grpc/GrpcTestBase.java Outdated Show resolved Hide resolved
...-clients/src/main/java/com/hedera/services/cli/signedstate/DumpContractStoresSubcommand.java Outdated Show resolved Hide resolved
hedera-node/hedera-app/src/main/java/com/hedera/node/app/Hedera.java Outdated Show resolved Hide resolved
hedera-node/hedera-app/src/main/java/com/hedera/node/app/HederaInjectionComponent.java Outdated Show resolved Hide resolved
...ode/hedera-app/src/main/java/com/hedera/node/app/grpc/impl/netty/NettyGrpcServerManager.java Show resolved Hide resolved
...m-sdk/swirlds-platform-core/src/main/java/com/swirlds/platform/state/spi/SchemaRegistry.java Outdated Show resolved Hide resolved
hedera-node/hedera-file-service/src/main/java/module-info.java Outdated Show resolved Hide resolved
hedera-node/hedera-app/src/test/java/com/hedera/node/app/services/ServiceRegistryImplTest.java Outdated Show resolved Hide resolved
hedera-node/hedera-app/src/main/java/com/hedera/node/app/OrderedServiceMigrator.java Outdated Show resolved Hide resolved
platform-sdk/swirlds-platform-core/src/main/java/com/swirlds/platform/state/spi/HapiUtils.java Outdated Show resolved Hide resolved
@imalygin imalygin dismissed stale reviews from jeromy-cannon and mhess-swl via f78f4bb May 31, 2024 22:49
@imalygin
chore: 13084 Separated ServiceGrpc and Service interface and used…
d8d541e 
… the latter where possible. Moved it and other classes to `swirlds-platform-core`

Signed-off-by: Ivan Malygin <ivan@swirldslabs.com>
@imalygin
Fixed a typo in the package name
c3a3ca3 
Signed-off-by: Ivan Malygin <ivan@swirldslabs.com>
@imalygin
Build check fixes
cf17dc5 
Signed-off-by: Ivan Malygin <ivan@swirldslabs.com>
alittley
alittley previously approved these changes Jun 3, 2024
View reviewed changes
jeromy-cannon
jeromy-cannon previously approved these changes Jun 3, 2024
View reviewed changes
@jeromy-cannon
Copy link
Contributor

@imalygin , instead of in the title chore:, perhaps refactor: would be a better fit.
012: Commit Title Conventions

@imalygin
Copy link
Member Author

imalygin commented Jun 4, 2024 •
edited

@imalygin , instead of in the title chore:, perhaps refactor: would be a better fit. 012: Commit Title Conventions

sure, that's a good idea, will do.

@imalygin imalygin changed the title chore: 13084 Split Service into Service and RpcService and migrated Service to platform-sdk refactoring: 13084 Split Service into Service and RpcService and migrated Service to platform-sdk Jun 4, 2024
@imalygin imalygin changed the title refactoring: 13084 Split Service into Service and RpcService and migrated Service to platform-sdk refactor: 13084 Split Service into Service and RpcService and migrated Service to platform-sdk Jun 4, 2024
@imalygin
Renamed ServiceFactory to RpcServiceFactory
5e4df35 
Signed-off-by: Ivan Malygin <ivan@swirldslabs.com>
@imalygin imalygin dismissed stale reviews from jeromy-cannon and alittley via 5e4df35 June 4, 2024 14:57
david-bakin-sl
david-bakin-sl approved these changes Jun 4, 2024
View reviewed changes
Copy link
Member

@david-bakin-sl david-bakin-sl left a comment

Choose a reason for hiding this comment

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

LGTM w.r.t. smart contract service changes only - trivial necessary import statement modifications + one file that corrected two @NotNull annotations.

@codecov Codecov
Copy link

codecov bot commented Jun 4, 2024 •
edited

Codecov Report

Attention: Patch coverage is 57.14286% with 9 lines in your changes missing coverage. Please review.

Project coverage is 67.57%. Comparing base (3bfe8f9) to head (5e4df35).
Report is 6 commits behind head on develop.

Files Patch % Lines
...src/main/java/com/swirlds/state/spi/HapiUtils.java 81.81% 1 Missing and 1 partial ⚠️
...a/node/app/service/consensus/ConsensusService.java 0.00% 1 Missing ⚠️
.../com/hedera/node/app/service/file/FileService.java 0.00% 1 Missing ⚠️
...a/node/app/service/networkadmin/FreezeService.java 0.00% 1 Missing ⚠️
.../node/app/service/networkadmin/NetworkService.java 0.00% 1 Missing ⚠️
...era/node/app/service/schedule/ScheduleService.java 0.00% 1 Missing ⚠️
...om/hedera/node/app/service/token/TokenService.java 0.00% 1 Missing ⚠️
.../com/hedera/node/app/service/util/UtilService.java 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@              Coverage Diff               @@
##             develop   #13331       +/-   ##
==============================================
+ Coverage           0   67.57%   +67.57%     
- Complexity         0    36387    +36387     
==============================================
  Files              0     3716     +3716     
  Lines              0   149461   +149461     
  Branches           0    15560    +15560     
==============================================
+ Hits               0   100998   +100998     
- Misses             0    44097    +44097     
- Partials           0     4366     +4366

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

tinker-michaelj
tinker-michaelj approved these changes Jun 4, 2024
View reviewed changes
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 for now, tyvm @imalygin ...we can look to update Services to keep application-only infrastructure needed for migration in the sharedValues map.

@nathanklick nathanklick merged commit 0ac344e into develop Jun 5, 2024
50 of 51 checks passed
@nathanklick nathanklick deleted the 13084-app-state branch June 5, 2024 02:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Neeharika-Sompalli Neeharika-Sompalli Neeharika-Sompalli left review comments

@jeromy-cannon jeromy-cannon jeromy-cannon approved these changes

@artemananiev artemananiev artemananiev approved these changes

@tinker-michaelj tinker-michaelj tinker-michaelj approved these changes

@alittley alittley alittley approved these changes

@mhess-swl mhess-swl mhess-swl approved these changes

@david-bakin-sl david-bakin-sl david-bakin-sl approved these changes

@kfa-aguda kfa-aguda Awaiting requested review from kfa-aguda kfa-aguda is a code owner automatically assigned from hashgraph/platform-hashgraph

@hendrikebbers hendrikebbers Awaiting requested review from hendrikebbers hendrikebbers is a code owner automatically assigned from hashgraph/platform-base

@tomzhenghedera tomzhenghedera Awaiting requested review from tomzhenghedera tomzhenghedera is a code owner automatically assigned from hashgraph/release-engineering

@derektriley derektriley Awaiting requested review from derektriley derektriley is a code owner automatically assigned from hashgraph/hedera-services

@jsync-swirlds jsync-swirlds Awaiting requested review from jsync-swirlds jsync-swirlds is a code owner automatically assigned from hashgraph/platform-architects

Assignees

@imalygin imalygin

Labels
None yet
Projects
None yet
Milestone
v0.52
Development

Successfully merging this pull request may close these issues.

Refactor Service and move it to platform-sdk
9 participants
@imalygin @mhess-swl @jeromy-cannon @artemananiev @tinker-michaelj @Neeharika-Sompalli @alittley @david-bakin-sl @nathanklick