Copilot commented on code in PR #13718:
URL: https://github.com/apache/ignite/pull/13718#discussion_r4334968366
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentClassLoader.java:
##########
@@ -610,6 +621,8 @@ private GridByteArrayList sendClassRequest(String name,
String path) throws Clas
List<IgniteException> clsReqExceptions = new ArrayList<>();
+ boolean notFound = false;
+
for (UUID nodeId : nodeListCp) {
Review Comment:
Logging a `WARN` for every participant that doesn’t have the class can
produce misleading noise in normal operation even when another node
successfully provides the class later in the loop. Consider lowering
intermediate-miss logs to DEBUG/INFO and emitting a single WARN only if *all*
participants fail (right before throwing), optionally including aggregated
details from the failed attempts.
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentClassLoader.java:
##########
@@ -632,21 +645,17 @@ private GridByteArrayList sendClassRequest(String name,
String path) throws Clas
if (res.success())
return res.byteSource();
else {
- // In case of shared resources/classes all nodes should
have it.
- String msg = "Failed to find class on remote node [class="
+ name + ", nodeId=" + node.id() +
- ", clsLdrId=" + ldrId + ", classLoadersHierarchy=" +
clsLdrHierarchy +
+ // Participants of a shared deployment are not guaranteed
to have the same class path (a node joins
+ // the deployment as soon as it is able to load one of its
classes), so keep asking the other nodes.
+ String msg = "Failed to find class on remote node (will
try other nodes, if any) [class=" + name +
+ ", nodeId=" + node.id() + ", clsLdrId=" + ldrId + ",
classLoadersHierarchy=" + clsLdrHierarchy +
", reason=" + res.errorMessage() + ']';
LT.warn(log, msg);
clsReqExceptions.add(new IgniteException(msg));
- synchronized (mux) {
- if (missedRsrcs != null)
- missedRsrcs.add(path);
- }
-
- break;
+ notFound = true;
Review Comment:
Logging a `WARN` for every participant that doesn’t have the class can
produce misleading noise in normal operation even when another node
successfully provides the class later in the loop. Consider lowering
intermediate-miss logs to DEBUG/INFO and emitting a single WARN only if *all*
participants fail (right before throwing), optionally including aggregated
details from the failed attempts.
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentClassLoader.java:
##########
@@ -807,16 +826,10 @@ else if (log.isDebugEnabled())
res.byteSource().size());
}
else {
- synchronized (mux) {
- // Cache unsuccessfully loaded resource.
- if (missedRsrcs != null)
- missedRsrcs.add(name);
- }
-
// Some frameworks like Spring often ask for the resources
// just in case - none will happen if there are no such
// resources. So we print out INFO level message.
- String msg = "Failed to get resource from node [" +
+ String msg = "Failed to get resource from node (will try
other nodes, if any) [" +
Review Comment:
Resource probing (e.g., frameworks asking for optional resources) can be
frequent; with this change, the code may log an INFO per node per miss,
multiplying log volume with participant count. To avoid excessive operational
noise, consider logging only once after all nodes fail (or lowering
intermediate misses to DEBUG), while still preserving the “try other nodes”
behavior.
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentClassLoader.java:
##########
@@ -825,8 +838,9 @@ else if (log.isDebugEnabled())
LT.info(log, msg);
- // Do not ask other nodes in case of shared mode all of
them should have the resource.
- return null;
+ // Participants of a shared deployment are not guaranteed
to have the same class path,
+ // so keep asking the other nodes.
+ notFound = true;
Review Comment:
Resource probing (e.g., frameworks asking for optional resources) can be
frequent; with this change, the code may log an INFO per node per miss,
multiplying log volume with participant count. To avoid excessive operational
noise, consider logging only once after all nodes fail (or lowering
intermediate misses to DEBUG), while still preserving the “try other nodes”
behavior.
##########
modules/core/src/test/java/org/apache/ignite/p2p/SharedDeploymentTest.java:
##########
@@ -81,6 +86,11 @@ public void testDeploymentFromSecondAndThird() throws
Exception {
for (Object o: res)
assertEquals(o, 42);
+ // In SHARED mode classes are undeployed only when all master
nodes leave: node 2 participates in the
+ // deployment serving the first version of the classes, so it has
to leave before node 3 is restarted
+ // with the second version. Otherwise node 3 would legitimately
share the already deployed classes.
+ stopGrid(2);
+
ignite3.close();
ignite3 = startGrid(3);
Review Comment:
Stopping a node and immediately restarting another node in tests can be
race-prone if topology/exchange and undeploy processing haven’t completed yet.
To reduce flakiness, add an `awaitPartitionMapExchange()` (or the equivalent
synchronization used in this test class) after `stopGrid(2)` (and/or after
closing node 3) before restarting node 3.
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/deployment/GridDeploymentClassLoader.java:
##########
@@ -782,6 +799,8 @@ else if (log.isDebugEnabled())
nodeLdrMapCp = singleNode ? nodeLdrMap : new HashMap<>(nodeLdrMap);
}
+ boolean notFound = false;
+
for (UUID nodeId : nodeListCp) {
Review Comment:
Resource probing (e.g., frameworks asking for optional resources) can be
frequent; with this change, the code may log an INFO per node per miss,
multiplying log volume with participant count. To avoid excessive operational
noise, consider logging only once after all nodes fail (or lowering
intermediate misses to DEBUG), while still preserving the “try other nodes”
behavior.
--
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]
