IGNITE-27542 : Use MessageSerializer for TcpDiscoveryMetricsUpdateMessage #12620
Conversation
…Message # Conflicts: # modules/core/src/main/java/org/apache/ignite/internal/managers/discovery/DiscoveryMessageFactory.java # modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/messages/TcpDiscoveryNodeMetricsMessage.java
Vladsz83
changed the title
|
| import org.apache.ignite.plugin.extensions.communication.Message; | ||
|
|
||
| /** Holds map of nodes metrics messages per node id. */ | ||
| public class TcpDiscoveryNodesMetricsMapMessage implements Message { |
There was a problem hiding this comment.
From usages of this class I can see that it is used solely for metrics from client nodes. I think we should capture this information right in the name of the class to make it easier for a developer to understand its purpose.
What do you think about a name like TcpDiscoveryClientNodesMetricsMessage?
In that case it may make sense to rewrite javadoc as well.
There was a problem hiding this comment.
I thought of it. Not sure. Where we use and what it can do are different. In the scope of this class, metrics of nodes are held. It can be either server metrics or client metrics. Would work with both.
Where it is actually used, the notation and/or javadoc 'client' or 'clients' is present.
...main/java/org/apache/ignite/spi/discovery/tcp/messages/TcpDiscoveryMetricsUpdateMessage.java
Outdated
Show resolved
Hide resolved
| assert metrics != null; | ||
| assert !this.metrics.containsKey(nodeId); | ||
| public void addServerMetrics(UUID srvrId, ClusterMetrics newMetrics) { | ||
| assert srvrId != null; |
There was a problem hiding this comment.
Do we really need to protect something in metric message with asserts? I believe that metrics are not that important, and even if some parameters are null, we could skip any actions with metrics without big harm to functionality of the cluster.
At the same time triggered assertion could cause a node or ever a cluster to shut down.
I don't think we should tie stability of the cluster to possible issues with metrics gathering. Importance of metrics is lower than an overall stability of the cluster.
There was a problem hiding this comment.
We still can drop a node by the metrics. At least a client. Look for 'Failing client node due to not receiving metrics updates '. It traverses a map. If key not found, node is considered as failed. Unfortunatelly. I would not touch this logic and what related to it. Instead, we might move these metrics into Communication. And/or remove them at all.
2 participants
No description provided.