Copilot commented on code in PR #7924:
URL: https://github.com/apache/ignite-3/pull/7924#discussion_r3029849755
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/metrics/LocalTopologyMetricsSource.java:
##########
@@ -111,28 +107,22 @@ protected class Holder implements
AbstractMetricSource.Holder<Holder> {
private final UuidGauge localNodeId = new UuidGauge(
"NodeId",
"Unique identifier of the local node",
- () -> physicalTopology.localMember().id());
+ localNode::id);
private final StringGauge localNodeName = new StringGauge(
"NodeName",
"Unique name of the local node",
- () -> physicalTopology.localMember().name());
+ localNode::name);
private final StringGauge networkAddress = new StringGauge(
"NetworkAddress",
"Network address of the local node",
- () -> {
- NetworkAddress addr =
physicalTopology.localMember().address();
- return addr != null ? addr.host() : "";
- });
+ () -> localNode.address().host());
private final IntGauge networkPort = new IntGauge(
"NetworkPort",
"Network port of the local node",
- () -> {
- NetworkAddress addr =
physicalTopology.localMember().address();
- return addr != null ? addr.port() : 0;
- });
+ () -> localNode.address().port());
Review Comment:
This removes the previous null-safety around `address()`. In tests and some
bootstrapping paths, `InternalClusterNode.address()` is sometimes constructed
as `null` (several tests in this PR still create `new ClusterNodeImpl(...,
null)`). This will cause `NullPointerException` when metrics are
registered/queried. Restore the null-guard (return empty host / 0 port) or
ensure `staticLocalNode()` is guaranteed to have a non-null address everywhere
it’s used for metrics.
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterManagementGroupManager.java:
##########
@@ -769,7 +769,7 @@ private void onElectedAsLeader(long term) {
// Send the ClusterStateMessage to all members of
the physical topology. We do not wait for the send operation
// because being unable to send ClusterState
messages should not fail the CMG service startup.
- InternalClusterNode thisNode =
topologyService.localMember();
+ InternalClusterNode thisNode =
clusterService.staticLocalNode();
Collection<InternalClusterNode> otherNodes =
topologyService.allMembers().stream()
.filter(node -> !thisNode.equals(node))
Review Comment:
Filtering `allMembers()` with `!thisNode.equals(node)` is no longer reliable
now that `staticLocalNode()` can return a different object instance/class than
`TopologyService` returns (e.g., `ScaleCubeClusterService`'s local node
wrapper). This can accidentally include the local node in `otherNodes` and
cause sending broadcast messages to self. Compare by stable identity instead
(e.g., `!thisNode.id().equals(node.id())` or by `name()`).
```suggestion
.filter(node ->
!thisNode.id().equals(node.id()))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
