Merged
Conversation
- Added gRPC service SupernodeService with GetStatus method to provide system status. - Created StatusRequest and StatusResponse messages to encapsulate request and response data. - Integrated CPU and memory metrics retrieval using gopsutil library in SupernodeStatusService. - Removed HealthCheck RPC from CascadeService and replaced it with a centralized status service. - Updated CascadeAdapter to utilize the new SupernodeService for status checks. - Implemented unit tests for SupernodeServer and SupernodeStatusService to ensure correct functionality. - Refactored existing health check logic into common service for better reusability.
- Removed redundant test files for supernode status and task services. - Implemented a new centralized SupernodeStatusService to manage system metrics and task information. - Introduced MetricsCollector for CPU and memory monitoring. - Created a unified StatusResponse structure to encapsulate service status details. - Developed a StorageHandler for P2P operations, including file storage and symbol management. - Enhanced task status management with a new Status type and associated methods. - Updated tests to reflect the new structure and ensure comprehensive coverage of service functionalities.
mateeullahmalik
requested review from
Copilot and
j-rafique
and removed request for
j-rafique
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a centralized status service for the Supernode, refactors shared code into base and storage packages, and updates cascade and SDK layers to use the new status API.
- Added
SupernodeStatusServicewith CPU/memory metrics and task aggregation viaTaskProvider. - Refactored
commonintobase(task/service helpers) andstorage(P2P storage handler), updating imports accordingly. - Replaced legacy
HealthCheckwithGetStatusacross cascade service, gRPC server, and SDK adapter; updated protobuf definitions and generated code.
Reviewed Changes
Copilot reviewed 35 out of 37 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| supernode/services/common/supernode/types.go | Defines StatusResponse, ServiceTasks, and TaskProvider |
| supernode/services/common/supernode/service.go | Implements SupernodeStatusService |
| supernode/services/common/supernode/metrics.go | Adds MetricsCollector for CPU/memory |
| supernode/services/common/base/supernode_task.go | Moves task helper into base and updates NewSuperNodeTask import |
| supernode/services/common/storage/handler.go | Moves storage handler into storage package |
| supernode/services/cascade/{status.go,service.go,...} | Updates cascade to use new status service and TaskProvider interface |
| sdk/adapters/supernodeservice/{types.go,adapter.go} | Updates SDK adapter to call GetStatus and fix type mappings |
| proto/supernode/{supernode.proto,action/cascade/service.proto} | Updates protobuf for status API; removes cascade HealthCheck RPC |
Comments suppressed due to low confidence (4)
sdk/adapters/supernodeservice/types.go:38
- [nitpick] The type name
SupernodeStatusresponsedoes not follow Go conventions. Rename toSupernodeStatusResponseto match exported naming patterns and related types.
type SupernodeStatusresponse struct {
supernode/services/common/supernode/metrics.go:23
- There are no unit tests covering error paths when
cpu.Percentfails. Consider adding tests that simulatecpu.Percentreturning an error to verify proper error logging and propagation.
func (m *MetricsCollector) CollectCPUMetrics(ctx context.Context) (usage, remaining string, err error) {
supernode/services/common/supernode/metrics.go:38
- The error path for
mem.VirtualMemoryis not covered in tests. Add a unit test that mocksmem.VirtualMemoryto return an error and asserts thatCollectMemoryMetricsreturns the error.
func (m *MetricsCollector) CollectMemoryMetrics(ctx context.Context) (total, used, available uint64, usedPerc float64, err error) {
supernode/services/common/base/supernode_task.go:57
- The code calls
task.New(...)but thetaskpackage is not imported. Addimport "github.com/LumeraProtocol/supernode/pkg/common/task"(or correct alias) so thattask.Newresolves correctly.
Task: task.New(common.StatusTaskStarted),
j-rafique
approved these changes
Jul 7, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.