-
Notifications
You must be signed in to change notification settings - Fork 4
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
chore: exposed a GRPC Service (endpoint) for the ServerStatus API #812
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Harsh Sawarkar <[email protected]>
Signed-off-by: Harsh Sawarkar <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first pass, I know is still in draft.
In General is missing a lot of Javadocs.
@@ -1,4 +1,4 @@ | |||
#·SPDX-License-Identifier:·Apache-2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like IDE style changes that should not be commited.
@@ -63,6 +66,7 @@ public BlockNodeApp( | |||
this.pbjBlockStreamService = requireNonNull(pbjBlockStreamService); | |||
this.pbjBlockAccessService = requireNonNull(pbjBlockAccessService); | |||
this.webServerBuilder = requireNonNull(webServerBuilder); | |||
this.pbjServerStatusService = pbjServerStatusService; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.pbjServerStatusService = pbjServerStatusService; | |
this.pbjServerStatusService = Objects.requireNonNull(pbjServerStatusService); |
public interface PbjServerStatusService extends ServiceInterface { | ||
enum ServerStatusMethod implements Method { | ||
serverStatus | ||
} | ||
|
||
@NonNull | ||
default String serviceName() { | ||
return SERVICE_NAME_BLOCK_NODE; | ||
} | ||
|
||
@NonNull | ||
default String fullName() { | ||
return FULL_SERVICE_NAME_BLOCK_NODE; | ||
} | ||
|
||
@NonNull | ||
default List<Method> methods() { | ||
return Arrays.asList(PbjServerStatusService.ServerStatusMethod.values()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing javadocs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
this.serviceStatus = serviceStatus; | ||
this.blockReader = blockReader; | ||
this.metricsService = metricsService; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing preConditions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
|
||
private final ServiceStatus serviceStatus; | ||
private final BlockReader<BlockUnparsed> blockReader; | ||
private final MetricsService metricsService; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we using the metrics?
We should be observing the amount of request we get, and if there were errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added usage.
() -> getBlockAsFileBlockStreamManager( | ||
getAbsoluteFolder("build/resources/test//BlockAsDirException/1/"))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needed change?
Signed-off-by: Harsh Sawarkar <[email protected]>
Reviewer Notes
PbjServerStatusServiceProxy
to expose server status api endpoint.Related Issue(s)
Fixes #777