IGNITE-4284 - ready for review

[Nikita Amelchev]

Hello. I fixed it.

Please, review.

https://issues.apache.org/jira/browse/IGNITE-4284 - Failed second client
node join with continuous query and peer class loading enabled

latest ci.tests:
http://ci.ignite.apache.org/project.html?projectId=IgniteTests&branch_IgniteTests=pull%2F1564%2Fhead


--
Best wishes,
Amelchev Nikita

[Alexey Goncharuk]

Nikita,

The fix looks wrong to me. The point of the assertion was to ensure an
invariant,
see org.apache.ignite.internal.processors.cache.query.continuous.CacheContinuousQueryManager#executeQuery
-V2 handler is created only when remote filter factory is not null.

The test observes both fields equal to null because deserialization failed
because peer class loading is not supported in certain places of code in
Ignite (namely, discovery subsystem), and joining node receives registered
continuous queries through discovery data. Silent ignorance of this failure
is wrong, we should either forbid such a node to start or support p2p class
loading for discovery (or suggest yet another solution).

2017-02-22 12:03 GMT+03:00 Nikita Amelchev <[hidden email]>:

> Hello. I fixed it.
>
> Please, review.
>
> https://issues.apache.org/jira/browse/IGNITE-4284 - Failed second client
> node join with continuous query and peer class loading enabled
>
> latest ci.tests:
> http://ci.ignite.apache.org/project.html?projectId=IgniteTests&branch_
> IgniteTests=pull%2F1564%2Fhead
>
>
> --
> Best wishes,
> Amelchev Nikita
>

[Nikita Amelchev]

Some about p2p class loading support. I thought that we can unmarshal on
processNodeAddFinishedMessage when discovery data recieved. But for
unmarshal required optimized marshaller(register classes in sys cache) that
starting after joinLatch countdown in this message. I tryed unmarshal
filters after joinLatch down but it broke some tests such as reconnects and
backups. Its possible unmarshal before joinLatch countdown and sys cache
was started? Or forbid node to start and throw checked exception?

2017-02-22 12:35 GMT+03:00 Alexey Goncharuk <[hidden email]>:

> Nikita,
>
> The fix looks wrong to me. The point of the assertion was to ensure an
> invariant,
> see org.apache.ignite.internal.processors.cache.query.continuous.
> CacheContinuousQueryManager#executeQuery
> -V2 handler is created only when remote filter factory is not null.
>
> The test observes both fields equal to null because deserialization failed
> because peer class loading is not supported in certain places of code in
> Ignite (namely, discovery subsystem), and joining node receives registered
> continuous queries through discovery data. Silent ignorance of this failure
> is wrong, we should either forbid such a node to start or support p2p class
> loading for discovery (or suggest yet another solution).
>
> 2017-02-22 12:03 GMT+03:00 Nikita Amelchev <[hidden email]>:
>
> > Hello. I fixed it.
> >
> > Please, review.
> >
> > https://issues.apache.org/jira/browse/IGNITE-4284 - Failed second client
> > node join with continuous query and peer class loading enabled
> >
> > latest ci.tests:
> > http://ci.ignite.apache.org/project.html?projectId=IgniteTests&branch_
> > IgniteTests=pull%2F1564%2Fhead
> >
> >
> > --
> > Best wishes,
> > Amelchev Nikita
> >
>

--
Best wishes,
Amelchev Nikita

[dkarachentsev]

Hi Nikita,

I've looked through the issue and, because fix is quite small and
important, committed to PR#1681 [1]. Please take a look at it. Here is
the code review [2].
I'll assign it to myself and resolve. If you have any questions about
fix, please ask.

Thanks for your effort!

[1] https://github.com/apache/ignite/pull/1681
[2] http://reviews.ignite.apache.org/ignite/review/IGNT-CR-140

-Dmitry.

22.03.2017 14:41, Nikita Amelchev пишет:

> Some about p2p class loading support. I thought that we can unmarshal on
> processNodeAddFinishedMessage when discovery data recieved. But for
> unmarshal required optimized marshaller(register classes in sys cache) that
> starting after joinLatch countdown in this message. I tryed unmarshal
> filters after joinLatch down but it broke some tests such as reconnects and
> backups. Its possible unmarshal before joinLatch countdown and sys cache
> was started? Or forbid node to start and throw checked exception?
>
> 2017-02-22 12:35 GMT+03:00 Alexey Goncharuk <[hidden email]>:
>
>> Nikita,
>>
>> The fix looks wrong to me. The point of the assertion was to ensure an
>> invariant,
>> see org.apache.ignite.internal.processors.cache.query.continuous.
>> CacheContinuousQueryManager#executeQuery
>> -V2 handler is created only when remote filter factory is not null.
>>
>> The test observes both fields equal to null because deserialization failed
>> because peer class loading is not supported in certain places of code in
>> Ignite (namely, discovery subsystem), and joining node receives registered
>> continuous queries through discovery data. Silent ignorance of this failure
>> is wrong, we should either forbid such a node to start or support p2p class
>> loading for discovery (or suggest yet another solution).
>>
>> 2017-02-22 12:03 GMT+03:00 Nikita Amelchev <[hidden email]>:
>>
>>> Hello. I fixed it.
>>>
>>> Please, review.
>>>
>>> https://issues.apache.org/jira/browse/IGNITE-4284 - Failed second client
>>> node join with continuous query and peer class loading enabled
>>>
>>> latest ci.tests:
>>> http://ci.ignite.apache.org/project.html?projectId=IgniteTests&branch_
>>> IgniteTests=pull%2F1564%2Fhead
>>>
>>>
>>> --
>>> Best wishes,
>>> Amelchev Nikita
>>>
>
>

[Nikita Amelchev]

Its ok. I doing it for long time because I create branch from master. And I
not saw ignite-4157 where appeared new events.

2017-03-28 16:59 GMT+03:00 Dmitry Karachentsev <[hidden email]>:

> Hi Nikita,
>
> I've looked through the issue and, because fix is quite small and
> important, committed to PR#1681 [1]. Please take a look at it. Here is the
> code review [2].
> I'll assign it to myself and resolve. If you have any questions about fix,
> please ask.
>
> Thanks for your effort!
>
> [1] https://github.com/apache/ignite/pull/1681
> [2] http://reviews.ignite.apache.org/ignite/review/IGNT-CR-140
>
> -Dmitry.
>
> 22.03.2017 14:41, Nikita Amelchev пишет:
>
> Some about p2p class loading support. I thought that we can unmarshal on
>> processNodeAddFinishedMessage when discovery data recieved. But for
>> unmarshal required optimized marshaller(register classes in sys cache)
>> that
>> starting after joinLatch countdown in this message. I tryed unmarshal
>> filters after joinLatch down but it broke some tests such as reconnects
>> and
>> backups. Its possible unmarshal before joinLatch countdown and sys cache
>> was started? Or forbid node to start and throw checked exception?
>>
>> 2017-02-22 12:35 GMT+03:00 Alexey Goncharuk <[hidden email]>:
>>
>> Nikita,
>>>
>>> The fix looks wrong to me. The point of the assertion was to ensure an
>>> invariant,
>>> see org.apache.ignite.internal.processors.cache.query.continuous.
>>> CacheContinuousQueryManager#executeQuery
>>> -V2 handler is created only when remote filter factory is not null.
>>>
>>> The test observes both fields equal to null because deserialization
>>> failed
>>> because peer class loading is not supported in certain places of code in
>>> Ignite (namely, discovery subsystem), and joining node receives
>>> registered
>>> continuous queries through discovery data. Silent ignorance of this
>>> failure
>>> is wrong, we should either forbid such a node to start or support p2p
>>> class
>>> loading for discovery (or suggest yet another solution).
>>>
>>> 2017-02-22 12:03 GMT+03:00 Nikita Amelchev <[hidden email]>:
>>>
>>> Hello. I fixed it.
>>>>
>>>> Please, review.
>>>>
>>>> https://issues.apache.org/jira/browse/IGNITE-4284 - Failed second
>>>> client
>>>> node join with continuous query and peer class loading enabled
>>>>
>>>> latest ci.tests:
>>>> http://ci.ignite.apache.org/project.html?projectId=IgniteTests&branch_
>>>> IgniteTests=pull%2F1564%2Fhead
>>>>
>>>>
>>>> --
>>>> Best wishes,
>>>> Amelchev Nikita
>>>>
>>>>
>>
>>
>

--
Best wishes,
Amelchev Nikita