You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "Vladsz83 (via GitHub)" <gi...@apache.org> on 2025/12/30 19:35:15 UTC
[PR] Test : Serialization of DiscoveryMetricsMsg [ignite]
Vladsz83 opened a new pull request, #12620:
URL: https://github.com/apache/ignite/pull/12620
(no comment)
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27542 : Use MessageSerializer for TcpDiscoveryMetricsUpdateMessage [ignite]
Posted by "Vladsz83 (via GitHub)" <gi...@apache.org>.
Vladsz83 commented on code in PR #12620:
URL: https://github.com/apache/ignite/pull/12620#discussion_r2698663331
##########
modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/messages/TcpDiscoveryNodesMetricsMapMessage.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.discovery.tcp.messages;
+
+import java.util.Map;
+import java.util.UUID;
+import org.apache.ignite.internal.Order;
+import org.apache.ignite.internal.managers.discovery.DiscoveryMessageFactory;
+import org.apache.ignite.internal.util.typedef.internal.S;
+import org.apache.ignite.plugin.extensions.communication.Message;
+
+/** Holds map of nodes metrics messages per node id. */
+public class TcpDiscoveryNodesMetricsMapMessage implements Message {
Review 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.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27542 : Use MessageSerializer for TcpDiscoveryMetricsUpdateMessage [ignite]
Posted by "sergey-chugunov-1985 (via GitHub)" <gi...@apache.org>.
sergey-chugunov-1985 merged PR #12620:
URL: https://github.com/apache/ignite/pull/12620
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27542 : Use MessageSerializer for TcpDiscoveryMetricsUpdateMessage [ignite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #12620:
URL: https://github.com/apache/ignite/pull/12620#issuecomment-3759813060
## [](https://sonarcloud.io/dashboard?id=apache_ignite&pullRequest=12620) **Quality Gate failed**
Failed conditions
 [14 New Code Smells](https://sonarcloud.io/dashboard?id=apache_ignite&pullRequest=12620) (required ≤ 1)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_ignite&pullRequest=12620)
##
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarQube for IDE](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27542 : Use MessageSerializer for TcpDiscoveryMetricsUpdateMessage [ignite]
Posted by "sergey-chugunov-1985 (via GitHub)" <gi...@apache.org>.
sergey-chugunov-1985 commented on code in PR #12620:
URL: https://github.com/apache/ignite/pull/12620#discussion_r2698700253
##########
modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/ServerImpl.java:
##########
@@ -3399,10 +3390,11 @@ private void sendMessageToClients(TcpDiscoveryAbstractMessage msg) {
for (ClientMessageWorker clientMsgWorker : clientMsgWorkers.values()) {
if (msgBytes == null) {
try {
- msgBytes = U.marshal(spi.marshaller(), msg);
+ msgBytes = clientMsgWorker.ses.serializeMessage(msg);
}
- catch (IgniteCheckedException e) {
- U.error(log, "Failed to marshal message: " + msg, e);
+ catch (IgniteCheckedException | IOException e) {
+ U.error(log, "Failed to serialize message to a client: " + msg + ", client id: "
Review Comment:
```suggestion
U.error(log, "Failed to serialize message: " + msg + ", recepient client id: "
```
##########
modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/TcpDiscoveryIoSession.java:
##########
@@ -320,4 +306,147 @@ private void detectSslAlert(byte firstByte, InputStream in) throws IOException {
if (hex.matches("15....00"))
throw new StreamCorruptedException("invalid stream header: " + hex);
}
+
+ /**
+ * Input stream allowing to read some data first as a prefix of an original input stream.
Review Comment:
```suggestion
* Input stream implementation that combines a byte array and a regular InputStream allowing to read
* bytes from the array first and then proceed with reading from InputStream.
```
##########
modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/TcpDiscoveryIoSession.java:
##########
@@ -320,4 +306,147 @@ private void detectSslAlert(byte firstByte, InputStream in) throws IOException {
if (hex.matches("15....00"))
throw new StreamCorruptedException("invalid stream header: " + hex);
}
+
+ /**
+ * Input stream allowing to read some data first as a prefix of an original input stream.
+ * Supports only basic read methods.
+ */
+ private static class PrefixedBufferedInputStream extends BufferedInputStream {
+ /** Prefix data input stream to read before the original input stream. */
+ @Nullable private ByteArrayInputStream prefixIs;
+
+ /** @param srcIs Original input stream to read when {@link #prefixIs} is empty. */
+ private PrefixedBufferedInputStream(InputStream srcIs) {
+ super(srcIs);
+ }
+
+ /** @param prefixData Prefix data to read before the original input stream. */
+ private void acceptPrefixBuffer(byte[] prefixData) {
Review Comment:
```suggestion
private void attachByteArray(byte[] attachment) {
```
##########
modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/ServerImpl.java:
##########
@@ -6073,9 +6066,11 @@ private void processMetricsUpdateMessage(TcpDiscoveryMetricsUpdateMessage msg) {
}
else {
// Message is on its second ring.
- removeMetrics(msg, locNodeId);
+ msg.removeServerMetrics(locNodeId);
- Collection<UUID> clientNodeIds = msg.clientNodeIds();
+ Collection<UUID> clientNodeIds = msg.clientNodeIds() == null
Review Comment:
Is `F.emptyIfNull` applicable here?
##########
modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/TcpDiscoveryIoSession.java:
##########
@@ -320,4 +306,147 @@ private void detectSslAlert(byte firstByte, InputStream in) throws IOException {
if (hex.matches("15....00"))
throw new StreamCorruptedException("invalid stream header: " + hex);
}
+
+ /**
+ * Input stream allowing to read some data first as a prefix of an original input stream.
+ * Supports only basic read methods.
+ */
+ private static class PrefixedBufferedInputStream extends BufferedInputStream {
+ /** Prefix data input stream to read before the original input stream. */
+ @Nullable private ByteArrayInputStream prefixIs;
Review Comment:
```suggestion
@Nullable private ByteArrayInputStream attachedBytesIs;
```
##########
modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/messages/TcpDiscoveryNodesMetricsMapMessage.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.discovery.tcp.messages;
+
+import java.util.Map;
+import java.util.UUID;
+import org.apache.ignite.internal.Order;
+import org.apache.ignite.internal.managers.discovery.DiscoveryMessageFactory;
+import org.apache.ignite.internal.util.typedef.internal.S;
+import org.apache.ignite.plugin.extensions.communication.Message;
+
+/** Holds map of nodes metrics messages per node id. */
+public class TcpDiscoveryNodesMetricsMapMessage implements Message {
Review Comment:
But at the same time we use it only for collecting and distributing client metrics, and there are no plans to develop discovery-based metrics further.
So I still think giving this class a more specific wouldn't harm existing codebase but would make it more readable instead.
There are five other classes matching "TcpDiscovery*MetricsMessage", and it is already hard to build a mental model of what's going on. Giving a sixth one a vague name makes this situation even worse.
##########
modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/TcpDiscoveryIoSession.java:
##########
@@ -320,4 +306,147 @@ private void detectSslAlert(byte firstByte, InputStream in) throws IOException {
if (hex.matches("15....00"))
throw new StreamCorruptedException("invalid stream header: " + hex);
}
+
+ /**
+ * Input stream allowing to read some data first as a prefix of an original input stream.
+ * Supports only basic read methods.
+ */
+ private static class PrefixedBufferedInputStream extends BufferedInputStream {
Review Comment:
```suggestion
private static class CompositeInputStream extends BufferedInputStream {
```
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27542 : Use MessageSerializer for TcpDiscoveryMetricsUpdateMessage [ignite]
Posted by "sergey-chugunov-1985 (via GitHub)" <gi...@apache.org>.
sergey-chugunov-1985 commented on code in PR #12620:
URL: https://github.com/apache/ignite/pull/12620#discussion_r2698386638
##########
modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/messages/TcpDiscoveryNodesMetricsMapMessage.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.spi.discovery.tcp.messages;
+
+import java.util.Map;
+import java.util.UUID;
+import org.apache.ignite.internal.Order;
+import org.apache.ignite.internal.managers.discovery.DiscoveryMessageFactory;
+import org.apache.ignite.internal.util.typedef.internal.S;
+import org.apache.ignite.plugin.extensions.communication.Message;
+
+/** Holds map of nodes metrics messages per node id. */
+public class TcpDiscoveryNodesMetricsMapMessage implements Message {
Review 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.
##########
modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/messages/TcpDiscoveryMetricsUpdateMessage.java:
##########
@@ -79,130 +83,136 @@ public TcpDiscoveryMetricsUpdateMessage(UUID creatorNodeId) {
/**
* Sets metrics for particular node.
*
- * @param nodeId Node ID.
- * @param metrics Node metrics.
+ * @param srvrId Server ID.
+ * @param newMetrics New server metrics to add.
*/
- public void setMetrics(UUID nodeId, ClusterMetrics metrics) {
- assert nodeId != null;
- assert metrics != null;
- assert !this.metrics.containsKey(nodeId);
+ public void addServerMetrics(UUID srvrId, ClusterMetrics newMetrics) {
+ assert srvrId != null;
Review 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.
##########
modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/messages/TcpDiscoveryMetricsUpdateMessage.java:
##########
@@ -79,130 +83,136 @@ public TcpDiscoveryMetricsUpdateMessage(UUID creatorNodeId) {
/**
* Sets metrics for particular node.
*
- * @param nodeId Node ID.
- * @param metrics Node metrics.
+ * @param srvrId Server ID.
+ * @param newMetrics New server metrics to add.
*/
- public void setMetrics(UUID nodeId, ClusterMetrics metrics) {
- assert nodeId != null;
- assert metrics != null;
- assert !this.metrics.containsKey(nodeId);
+ public void addServerMetrics(UUID srvrId, ClusterMetrics newMetrics) {
+ assert srvrId != null;
+ assert newMetrics != null;
+
+ if (serversFullMetricsMsgs == null)
+ serversFullMetricsMsgs = new HashMap<>();
+
+ assert !serversFullMetricsMsgs.containsKey(srvrId);
+
+ serversFullMetricsMsgs.compute(srvrId, (srvrId0, srvrFullMetrics) -> {
+ if (srvrFullMetrics == null)
+ srvrFullMetrics = new TcpDiscoveryNodeFullMetricsMessage();
+
+ srvrFullMetrics.nodeMetricsMessage(new TcpDiscoveryNodeMetricsMessage(newMetrics));
- this.metrics.put(nodeId, new MetricsSet(metrics));
+ return srvrFullMetrics;
+ });
}
/**
* Sets cache metrics for particular node.
*
- * @param nodeId Node ID.
- * @param metrics Node cache metrics.
+ * @param srvrId Server ID.
+ * @param newCachesMetrics News server's caches metrics to add.
*/
- public void setCacheMetrics(UUID nodeId, Map<Integer, CacheMetrics> metrics) {
- assert nodeId != null;
- assert metrics != null;
- assert !this.cacheMetrics.containsKey(nodeId);
+ public void addServerCacheMetrics(UUID srvrId, Map<Integer, CacheMetrics> newCachesMetrics) {
+ assert srvrId != null;
+ assert newCachesMetrics != null;
- if (!F.isEmpty(metrics))
- this.cacheMetrics.put(nodeId, metrics);
+ if (serversFullMetricsMsgs == null)
+ serversFullMetricsMsgs = new HashMap<>();
+
+ assert serversFullMetricsMsgs.containsKey(srvrId) && serversFullMetricsMsgs.get(srvrId).cachesMetricsMessages() == null;
+
+ serversFullMetricsMsgs.compute(srvrId, (srvrId0, srvrFullMetrics) -> {
+ if (srvrFullMetrics == null)
+ srvrFullMetrics = new TcpDiscoveryNodeFullMetricsMessage();
+
+ Map<Integer, CacheMetricsMessage> newCachesMsgsMap = U.newHashMap(newCachesMetrics.size());
+
+ newCachesMetrics.forEach((cacheId, cacheMetrics) ->
+ newCachesMsgsMap.put(cacheId, new TcpDiscoveryCacheMetricsMessage(cacheMetrics)));
+
+ srvrFullMetrics.cachesMetricsMessages(newCachesMsgsMap);
+
+ return srvrFullMetrics;
+ });
}
/**
- * Sets metrics for a client node.
+ * Sets metrics for a connected client node.
*
- * @param nodeId Server node ID.
- * @param clientNodeId Client node ID.
- * @param metrics Node metrics.
+ * @param srvrId Server node ID.
+ * @param clientNodeId Connected client node ID.
+ * @param clientMetrics Client metrics.
*/
- public void setClientMetrics(UUID nodeId, UUID clientNodeId, ClusterMetrics metrics) {
- assert nodeId != null;
+ public void setClientMetrics(UUID srvrId, UUID clientNodeId, ClusterMetrics clientMetrics) {
Review Comment:
```suggestion
public void addClientMetrics(UUID srvrId, UUID clientNodeId, ClusterMetrics clientMetrics) {
```
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27542 : Use MessageSerializer for TcpDiscoveryMetricsUpdateMessage [ignite]
Posted by "Vladsz83 (via GitHub)" <gi...@apache.org>.
Vladsz83 commented on code in PR #12620:
URL: https://github.com/apache/ignite/pull/12620#discussion_r2698644442
##########
modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/messages/TcpDiscoveryMetricsUpdateMessage.java:
##########
@@ -79,130 +83,136 @@ public TcpDiscoveryMetricsUpdateMessage(UUID creatorNodeId) {
/**
* Sets metrics for particular node.
*
- * @param nodeId Node ID.
- * @param metrics Node metrics.
+ * @param srvrId Server ID.
+ * @param newMetrics New server metrics to add.
*/
- public void setMetrics(UUID nodeId, ClusterMetrics metrics) {
- assert nodeId != null;
- assert metrics != null;
- assert !this.metrics.containsKey(nodeId);
+ public void addServerMetrics(UUID srvrId, ClusterMetrics newMetrics) {
+ assert srvrId != null;
Review 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.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org