chore: refactored the ServiceStatus interface methods to a new interface #801

HarshSawarkar · 2025-03-07T14:34:04Z

Reviewer Notes

Moved methods specific to the health of the web server to the new WebServerStatus interface.
Modifications to the dependents of this method were made to ensure backward compatibility.

Related Issue(s)

Fixes #575

Signed-off-by: Harsh Sawarkar <[email protected]>

Nana-EC · 2025-03-07T16:34:16Z

Thanks @HarshSawarkar for the contribution, welcome 🎊 .
@AlfredoG87 and others will review the PR and will provide comments.
Based on that we'll see if your contribution can be included in this sprint schedule.
Exciting 🦾

codecov · 2025-03-07T16:39:30Z

Codecov Report

Attention: Patch coverage is 67.34694% with 16 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...dera/block/server/service/WebServerStatusImpl.java	47.36%	10 Missing ⚠️
.../block/server/mediator/LiveStreamMediatorImpl.java	33.33%	1 Missing and 1 partial ⚠️
...com/hedera/block/server/notifier/NotifierImpl.java	33.33%	1 Missing and 1 partial ⚠️
...a/block/server/pbj/PbjBlockStreamServiceProxy.java	33.33%	2 Missing ⚠️

❌ Your patch check has failed because the patch coverage (67.34%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

@@             Coverage Diff              @@
##               main     #801      +/-   ##
============================================
+ Coverage     84.59%   84.66%   +0.06%     
- Complexity      640      641       +1     
============================================
  Files           130      131       +1     
  Lines          2960     2973      +13     
  Branches        208      208              
============================================
+ Hits           2504     2517      +13     
  Misses          388      388              
  Partials         68       68

Files with missing lines	Coverage Δ	Complexity Δ
...ain/java/com/hedera/block/server/BlockNodeApp.java	`100.00% <100.00%> (ø)`	`3.00 <0.00> (ø)`
.../hedera/block/server/health/HealthServiceImpl.java	`100.00% <100.00%> (ø)`	`7.00 <5.00> (ø)`
...ock/server/mediator/LiveStreamMediatorBuilder.java	`81.81% <100.00%> (+1.81%)`	`3.00 <3.00> (ø)`
...block/server/mediator/MediatorInjectionModule.java	`100.00% <100.00%> (ø)`	`2.00 <0.00> (ø)`
...a/block/server/pbj/PbjBlockAccessServiceProxy.java	`100.00% <100.00%> (ø)`	`9.00 <0.00> (ø)`
...server/persistence/PersistenceInjectionModule.java	`71.42% <ø> (ø)`	`10.00 <0.00> (ø)`
...rver/persistence/StreamPersistenceHandlerImpl.java	`71.59% <100.00%> (+0.32%)`	`10.00 <2.00> (ø)`
...ock/server/producer/ProducerBlockItemObserver.java	`79.52% <100.00%> (+0.16%)`	`21.00 <0.00> (ø)`
...hedera/block/server/service/ServiceStatusImpl.java	`81.81% <ø> (+29.81%)`	`4.00 <0.00> (-2.00)`	⬆️
...er/verification/StreamVerificationHandlerImpl.java	`100.00% <100.00%> (ø)`	`4.00 <2.00> (ø)`
... and 4 more

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

HarshSawarkar · 2025-03-07T16:57:59Z

Thanks @HarshSawarkar for the contribution, welcome 🎊 . @AlfredoG87 and others will review the PR and will provide comments. Based on that we'll see if your contribution can be included in this sprint schedule. Exciting 🦾

Thanks, @Nana-EC ! I’ll be more than happy to resolve any comments received from the team. I’m excited and looking forward to making more contributions. By the way, is there a Discord channel or group to connect with in case I have any doubts or need help?

Signed-off-by: Harsh Sawarkar <[email protected]>

AlfredoG87

Overall looks good, leave some comments and suggestions, mostly nits.

However PR is failing checks, please let me know if you have issues with running the tests. I will do a second pass once all Tests are passing.

Thanks!

AlfredoG87 · 2025-03-10T22:33:17Z

.github/ISSUE_TEMPLATE/03-Bug-Template.yml

@@ -1,4 +1,4 @@
-#·SPDX-License-Identifier:·Apache-2.0


was this change made by spotless? otherwise I would recommend not changing files that don't need to be changed.

if it was spotlessApply, then maybe we can consider the unrelated format change.

This is a bad characters fix (and much appreciated in this case).
The · somehow got copied into the YAML file, and it makes the SPDX identifier not process correctly.
I assume spotlessApply made the change, but it could have just been an eagle-eyed review.

Thanks for the clarification @jsync-swirlds

AlfredoG87 · 2025-03-10T22:35:41Z

server/src/main/java/com/hedera/block/server/BlockNodeApp.java

            @NonNull final HealthService healthService,
            @NonNull final PbjBlockStreamService pbjBlockStreamService,
            @NonNull final PbjBlockAccessService pbjBlockAccessService,
            @NonNull final WebServerConfig.Builder webServerBuilder,
            @NonNull final ServerConfig serverConfig,
            @NonNull final ConfigurationLogging configurationLogging) {
        this.serviceStatus = requireNonNull(serviceStatus);
+        this.webServerStatus = webServerStatus;


Suggested change

this.webServerStatus = webServerStatus;

this.webServerStatus = requireNonNull(webServerStatus);

In general we use preconditions on constructors to fail fast.

AlfredoG87 · 2025-03-10T22:40:46Z

server/src/main/java/com/hedera/block/server/service/WebServerStatusInjectionModule.java

+// SPDX-License-Identifier: Apache-2.0
+package com.hedera.block.server.service;
+
+import dagger.Binds;
+import dagger.Module;
+import javax.inject.Singleton;
+
+/** A Dagger module for providing dependencies for WebServerStatus Module. */
+@Module
+public interface WebServerStatusInjectionModule {
+
+    /**
+     * Binds the service status to the Web Server status implementation.
+     *
+     * @param webServerStatus needs a service status implementation
+     * @return the service status implementation
+     */
+    @Singleton
+    @Binds
+    WebServerStatus bindWebServerStatus(WebServerStatusImpl webServerStatus);
+}


We try to keep a single InjectionModule file per module in this case com.hedera.block.server.service package.

We can add this Binding in existing file: ServiceInjectionModule

AlfredoG87 · 2025-03-10T22:47:49Z

server/src/main/java/com/hedera/block/server/BlockNodeAppInjectionComponent.java

@@ -22,6 +23,7 @@
        modules = {
            NotifierInjectionModule.class,
            ServiceInjectionModule.class,
+            WebServerStatusInjectionModule.class,


Do we want to add a new InjectionModule?

AlfredoG87 · 2025-03-10T22:50:26Z

server/src/main/java/com/hedera/block/server/pbj/PbjBlockStreamServiceProxy.java

@@ -106,6 +109,7 @@ public PbjBlockStreamServiceProxy(
            @NonNull final ProducerConfig producerConfig) {

        this.serviceStatus = Objects.requireNonNull(serviceStatus);
+        this.webServerStatus = webServerStatus;


Suggested change

this.webServerStatus = webServerStatus;

this.webServerStatus = Objects.requireNonNull(webServerStatus);

AlfredoG87 · 2025-03-10T22:51:09Z

server/src/main/java/com/hedera/block/server/pbj/PbjBlockAccessServiceProxy.java

        this.serviceStatus = serviceStatus;
+        this.webServerStatus = webServerStatus;
        this.blockReader = blockReader;
        this.metricsService = metricsService;


Suggested change

this.serviceStatus = serviceStatus;

this.webServerStatus = webServerStatus;

this.blockReader = blockReader;

this.metricsService = metricsService;

this.serviceStatus = Objects.requireNonNull(serviceStatus);

this.webServerStatus = Objects.requireNonNull(webServerStatus);

this.blockReader = Objects.requireNonNull(blockReader);

this.metricsService = Objects.requireNonNull(metricsService);

AlfredoG87 · 2025-03-10T22:51:48Z

server/src/main/java/com/hedera/block/server/notifier/NotifierImpl.java

        this.mediator = mediator;
        this.metricsService = metricsService;
        this.serviceStatus = serviceStatus;
+        this.webServerStatus = webServerStatus;


Suggested change

this.mediator = mediator;

this.metricsService = metricsService;

this.serviceStatus = serviceStatus;

this.webServerStatus = webServerStatus;

this.mediator = Objects.requireNonNull(mediator);

this.metricsService = Objects.requireNonNull(metricsService);

this.serviceStatus = Objects.requireNonNull(serviceStatus);

this.webServerStatus = Objects.requireNonNull(webServerStatus);

AlfredoG87 · 2025-03-10T22:52:11Z

server/src/main/java/com/hedera/block/server/mediator/LiveStreamMediatorImpl.java

        this.metricsService = metricsService;
+        this.webServerStatus = webServerStatus;


Suggested change

this.metricsService = metricsService;

this.webServerStatus = webServerStatus;

this.metricsService = Objects.requireNonNull(metricsService);

this.webServerStatus = Objects.requireNonNull(webServerStatus);

AlfredoG87 · 2025-03-10T22:52:33Z

server/src/main/java/com/hedera/block/server/health/HealthServiceImpl.java

-    public HealthServiceImpl(@NonNull ServiceStatus serviceStatus) {
-        this.serviceStatus = serviceStatus;
+    public HealthServiceImpl(@NonNull WebServerStatus webServerStatus) {
+        this.webServerStatus = webServerStatus;


Suggested change

this.webServerStatus = webServerStatus;

this.webServerStatus = Objects.requireNonNull(webServerStatus);

AlfredoG87 · 2025-03-10T22:54:37Z

...or/src/test/java/com/hedera/block/simulator/generator/BlockAsFileBlockStreamManagerTest.java

+                () -> getBlockAsFileBlockStreamManager(
+                        getAbsoluteFolder("build/resources/test//BlockAsDirException/1/")));
    }


was this also made by spotlessApply? otherwise better not to commit this auto changes by some IDE style parser.
We use the included spotless plugin to enforce format and style checks

Yes, it was made by spotlessApply, so I’m keeping it intact.

andrewb1269hg

Review and approve .github/ISSUE_TEMPLATE/03-Bug-Template.yml

Signed-off-by: Harsh Sawarkar <[email protected]>

jsync-swirlds · 2025-03-11T17:03:56Z

Just an FYI, the reason basically every file conflicts now is that we did a rename of all packages to reflect the org.hiero naming.

Refactored the ServiceStatus interface methods to a new interface.

fc0bc21

Signed-off-by: Harsh Sawarkar <[email protected]>

HarshSawarkar requested review from a team as code owners March 7, 2025 14:34

HarshSawarkar changed the title ~~chore: refactored the ServiceStatus interface methods to a new interface.~~ chore: refactored the ServiceStatus interface methods to a new interface Mar 8, 2025

Resolved conflicts.

2afe4da

Signed-off-by: Harsh Sawarkar <[email protected]>

AlfredoG87 reviewed Mar 10, 2025

View reviewed changes

andrewb1269hg previously approved these changes Mar 11, 2025

View reviewed changes

Fixed tests.

22eccf7

Signed-off-by: Harsh Sawarkar <[email protected]>

HarshSawarkar dismissed andrewb1269hg’s stale review via 22eccf7 March 11, 2025 06:14

HarshSawarkar requested a review from AlfredoG87 March 11, 2025 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: refactored the ServiceStatus interface methods to a new interface #801

chore: refactored the ServiceStatus interface methods to a new interface #801

HarshSawarkar commented Mar 7, 2025

Nana-EC commented Mar 7, 2025

codecov bot commented Mar 7, 2025

HarshSawarkar commented Mar 7, 2025

AlfredoG87 left a comment •

edited

Loading

AlfredoG87 Mar 10, 2025

AlfredoG87 Mar 10, 2025

jsync-swirlds Mar 11, 2025

AlfredoG87 Mar 11, 2025

AlfredoG87 Mar 10, 2025

HarshSawarkar Mar 11, 2025

AlfredoG87 Mar 10, 2025

HarshSawarkar Mar 11, 2025

AlfredoG87 Mar 10, 2025

AlfredoG87 Mar 10, 2025

AlfredoG87 Mar 10, 2025

AlfredoG87 Mar 10, 2025

AlfredoG87 Mar 10, 2025

AlfredoG87 Mar 10, 2025

AlfredoG87 Mar 10, 2025

HarshSawarkar Mar 11, 2025

andrewb1269hg left a comment

jsync-swirlds commented Mar 11, 2025

	this.webServerStatus = webServerStatus;
	this.webServerStatus = requireNonNull(webServerStatus);

	this.webServerStatus = webServerStatus;
	this.webServerStatus = Objects.requireNonNull(webServerStatus);

		this.metricsService = metricsService;
		this.webServerStatus = webServerStatus;

chore: refactored the ServiceStatus interface methods to a new interface #801

Are you sure you want to change the base?

chore: refactored the ServiceStatus interface methods to a new interface #801

Conversation

HarshSawarkar commented Mar 7, 2025

Reviewer Notes

Related Issue(s)

Nana-EC commented Mar 7, 2025

codecov bot commented Mar 7, 2025

Codecov Report

HarshSawarkar commented Mar 7, 2025

AlfredoG87 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewb1269hg left a comment

Choose a reason for hiding this comment

jsync-swirlds commented Mar 11, 2025

AlfredoG87 left a comment •

edited

Loading