[
https://issues.apache.org/jira/browse/IGNITE-29181?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18130194#comment-18130194
]
Nikolay Izhikov commented on IGNITE-29181:
------------------------------------------
Review of PR #13718.
Verified locally on the PR branch: the 5 new tests pass, the request-recording
reproducer attached to this ticket passes,
{{GridCacheAtomicEntryProcessorDeploymentSelfTest}} (IGNITE-20861),
{{GridP2PSameClassLoaderSelfTest}}, hot redeploy, undeploy, missed-resource,
continuous and double-deployment P2P tests pass, checkstyle is clean.
Findings:
# *SHARED-mode behaviour change needs an explicit sign-off.* The PR adds
{{stopGrid(2)}} to {{SharedDeploymentTest}}; without it the test fails on the
PR code (checked: the master version of the test expects 43 and gets 42). A
node that restarts with changed bytecode under the same user version now
receives the old class from a still-alive participant. On 2.16 and on master it
got its own deployment and the new class. This matches the documented SHARED
contract, but it is a user-visible change and should be stated in the PR
description and release notes rather than left inside a test edit. The original
report describes different applications with overlapping class names, which is
exactly where reuse by a single class name can silently serve the wrong
bytecode. That risk already exists on master, so it is not a regression of this
PR, but worth mentioning here.
# *Reverse-order scenario is untested.* In
{{testNeighbourClientWithoutJobClass}} the full client deploys first, so the
owner is already first in the node list and the new fall-through logic in
{{sendClassRequest}} never runs. With the partial client first, the fix depends
entirely on "keep asking other nodes", and the existing log-listener assertion
would fail in that order since the miss is logged. Proposed test below: it
passes on the PR code and fails on master. Patch attached
({{IGNITE-29181-asked-first-test.patch}}).
# *Warning noise.* Every fall-through logs "Failed to find class on remote
node" at WARN, once per class per non-owning participant. On clusters where
many clients share one deployment this is loud for a normal path. Suggest DEBUG
or INFO while nodes remain, and WARN only in the final "not found anywhere"
branch.
# *Probing cost deserves a sentence in the description.* A class unknown to the
local node from a new sender now probes every existing deployment of that user
version over the network before a new deployment is created, and the per-loader
miss cache is cleared on each {{register}}. Master's IGNITE-20861 code did the
same under the lock, so this is an improvement, but it should be documented.
Verdict: approve once item 1 is acknowledged in the ticket and PR description.
Items 3 and 4 are non-blocking.
Proposed test for item 2, to be added to
{{org.apache.ignite.p2p.GridP2PClientDeploymentSourceTest}}:
{code:java}
/**
* Same as {@link #testNeighbourClientWithoutJobClass()} but the clients
deploy in the opposite order, so the client
* lacking the job class is the first participant of the shared deployment
and is asked first:
* <ol>
* <li>Client 2 (has task 2 only) executes task 2 on the server and
creates the shared deployment.</li>
* <li>Client 1 (has task 1, job 1 and task 2) executes task 2 and
joins that deployment after client 2.</li>
* <li>Client 1 executes task 1. The server asks client 2 for job 1
first, gets "not found" and must go on
* to client 1 instead of giving up on the first miss.</li>
* </ol>
*/
@Test
public void testNeighbourClientWithoutJobClassAskedFirst() throws Exception
{
depMode = SHARED;
IgniteEx srv = startGrid(SRV);
IgniteEx fullCli = startClientGrid(FULL_CLI);
IgniteEx partialCli = startClientGrid(PARTIAL_CLI);
UUID srvId = srv.localNode().id();
String partialCliId = partialCli.localNode().id().toString();
// The log abbreviates packages, so match on the simple names. The
trailing comma keeps the task name from
// matching its job.
String task1SimpleName =
TASK1_CLS_NAME.substring(TASK1_CLS_NAME.lastIndexOf('.') + 1);
String job1SimpleName =
JOB1_CLS_NAME.substring(JOB1_CLS_NAME.lastIndexOf('.') + 1);
// Client 2 has neither task 1 nor job 1. The server reports each miss
on client 2 exactly once and then
// goes on to the next node.
LogListener taskMissedOnPartialLsnr = LogListener.matches(s ->
s.contains(CLS_NOT_FOUND_MSG)
&& s.contains(task1SimpleName + ',')
&& s.contains(partialCliId)).times(1).build();
LogListener jobMissedOnPartialLsnr = LogListener.matches(s ->
s.contains(CLS_NOT_FOUND_MSG)
&& s.contains(job1SimpleName + ',')
&& s.contains(partialCliId)).times(1).build();
// No other class is ever reported missing: task 2 is served by client
2, task 1 and job 1 by client 1.
LogListener otherMissLsnr = LogListener.matches(s ->
s.contains(CLS_NOT_FOUND_MSG)
&& !s.contains(task1SimpleName)).build();
srvLog.registerListener(taskMissedOnPartialLsnr);
srvLog.registerListener(jobMissedOnPartialLsnr);
srvLog.registerListener(otherMissLsnr);
Class task2Partial = partialLdr.loadClass(TASK2_CLS_NAME);
Class task2Full = fullLdr.loadClass(TASK2_CLS_NAME);
Class task1Full = fullLdr.loadClass(TASK1_CLS_NAME);
Integer ldr2Partial =
(Integer)partialCli.compute().execute(task2Partial, srvId);
Integer ldr2Full = (Integer)fullCli.compute().execute(task2Full, srvId);
// Client 1 joined the deployment created by client 2.
assertEquals(ldr2Partial, ldr2Full);
// Job 1 exists on client 1 only. The server asks client 2 first and
must fall through to client 1.
Integer ldr1Full = (Integer)fullCli.compute().execute(task1Full, srvId);
assertEquals(ldr2Partial, ldr1Full);
assertTrue("Task class was not requested from the first participant
before the owner",
taskMissedOnPartialLsnr.check());
assertTrue("Job class was not requested from the first participant
before the owner",
jobMissedOnPartialLsnr.check());
assertFalse("A class other than the job class was requested from a node
that does not have it",
otherMissLsnr.check());
}
{code}
> P2P: in SHARED mode a job class is requested from a neighbour client that
> does not have it (regression of IGNITE-20861)
> -----------------------------------------------------------------------------------------------------------------------
>
> Key: IGNITE-29181
> URL: https://issues.apache.org/jira/browse/IGNITE-29181
> Project: Ignite
> Issue Type: Bug
> Affects Versions: 2.17
> Reporter: Nikolay Izhikov
> Assignee: Evgeny Stanilovsky
> Priority: Major
> Labels: p2p
> Fix For: 2.19
>
> Attachments: GridP2PClientDeploymentSourceTest.java,
> IGNITE-29181-asked-first-test.patch
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> Since IGNITE-20861 (2.17) a server in SHARED deployment mode can request a
> job class from a client node that does not have it, even though the client
> that submitted the task has it. The peer class loader gives up on the first
> node that reports the class missing, so the job fails.
> h3. Scenario
> Thick clients with partly overlapping class paths (different applications on
> each client):
> * Client A has task {{T1}} with job {{J1}}, and task {{T2}}.
> * Client B has task {{T2}} with its job, and may have {{T1}} but *not* {{J1}}.
> * Peer class loading enabled, SHARED mode (default).
> # A executes {{T2}}. The server creates a shared deployment {{D_A}} whose
> peer loader points at A.
> # B executes {{T2}}. In {{GridDeploymentPerVersionStore#getDeployment}} the
> block added by IGNITE-20861 scans every existing shared deployment and calls
> {{dep0.deployedClass(T2)}} on {{D_A}}. {{D_A}} loads {{T2}} from A, so B is
> registered as a participant of {{D_A}} with
> {{GridDeploymentClassLoader#register}}, which puts B *first* in the loader's
> node list.
> # A executes {{T1}}. {{GridDeploymentManager#getGlobalDeployment}} resolves
> A's request through {{searchDeploymentCache}} to {{D_A}} (A is its
> participant). {{D_A}}'s loader asks B first for {{T1}} and then for {{J1}}. B
> does not have {{J1}}, replies "not found", and
> {{GridDeploymentClassLoader#sendClassRequest}} breaks out of the node loop
> ("in case of shared resources/classes all nodes should have it") and caches
> the class as missed. The job fails with {{Failed to deserialize task
> attributes}} / {{P2PClassNotFoundException}}.
> In 2.16 every submitting node got a shared deployment of its own in this
> case, so {{D_A}}'s loader only ever asked A.
> Observed on a production cluster: "Ignite goes for the job code not to the
> initiator node, which has the classes, but to a neighbouring client node that
> has the task class but not the job class."
> h3. Reproducer
> {{GridP2PClientDeploymentSourceTest}} (attached, goes to
> {{org.apache.ignite.internal.managers.deployment}} because
> {{GridDeploymentRequest}} accessors are package-private; register it in
> {{IgniteP2PSelfTestSuite}}). It records every outgoing
> {{GridDeploymentRequest}} with its destination through
> {{TestRecordingCommunicationSpi}} and asserts that classes are requested only
> from nodes that hold them.
> * {{testNeighbourClientWithoutJobClass}}: one server, client A with all
> classes of {{ignite-extdata-p2p}}, client B whose loader excludes
> {{SingleSplitTestTask$SingleSplitTestJob}} and
> {{GridSingleSplitTestJobTarget}}. A runs {{P2PTestTaskExternalPath1}}, B runs
> {{P2PTestTaskExternalPath2}}, A runs {{SingleSplitTestTask}}.
> * {{testSharedMode}} / {{testContinuousMode}} / {{testPrivateMode}} /
> {{testIsolatedMode}}: two servers, two clients with the classes (separate
> loaders), two clients without; every request must target a client that has
> the classes, also after the first owner leaves.
> Bisect:
> ||commit||testNeighbourClientWithoutJobClass||other 4||
> |531eba2c86e (parent of IGNITE-20861)|pass|pass|
> |be246366346 IGNITE-20861|fail: {{SingleSplitTestJob}} requested from B|pass|
> |master|fail|pass|
> |master with the IGNITE-20861 block in {{getDeployment}} disabled|pass|pass|
> Log on the failing side:
> {noformat}
> Failed to find class on remote node
> [class=org.apache.ignite.tests.p2p.classic.SingleSplitTestTask$SingleSplitTestJob,
> nodeId=<client B>, clsLdrId=<client B loader>, ...]
> Remote job threw user exception ...: Failed to deserialize task attributes
> [taskName=org.apache.ignite.tests.p2p.classic.SingleSplitTestTask, ...,
> taskClsLdr=GridDeploymentClassLoader [..., nodeLdrMap=HashMap {<client
> B>=..., <client A>=...}, ...]]
> {noformat}
> h3. Possible fixes
> * Do not register a sender into a shared deployment it did not create just
> because that deployment can load the task class: a node holding the task
> class does not necessarily hold every class of the deployment.
> * Or make {{GridDeploymentClassLoader#sendClassRequest}} continue with the
> next participant when one reports the class missing instead of breaking and
> caching the miss.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
