Danger (?) change of DiscoveryCustomEvent in GridDhtPartitionsExchangeFuture#onDone

[daradurvs]

Hi Igniters!

I think I found an illegal behavior in
GridDhtPartitionsExchangeFuture#onDone, the following code is called
here:
((DiscoveryCustomEvent)firstDiscoEvt).customMessage(null);

That means a global instance of 'DiscoveryCustomEvent' is being
mutated outside discovery-spi infrastructure. It also means that
discovery listeners receive 'DiscoveryCustomEvent' with 'null' field
instead of 'CustomMessage' which they may rely on.

Could someone confirm if it is wrong behavior and should be fixed?

--
Best Regards, Vyacheslav D.

[daradurvs]

I think that I understand a reason for doing this:
The most custom events which handle in
'GridDhtPartitionsExchangeFuture' are using only in PME flow and
reason is release them for GC as soon as possible.

But there are some other systems which can listen to the same events,
for example, to perform activation/deactivation actions them should
handle [ChangeGlobalStateMessage, ChangeGlobalStateFinishMessage]
which can be reset to 'null' by PME earlier then they will be handled
by other systems.

I'd suggest do not reset to 'null' custom messages in
'DiscoveryCustomEvent ' (at least without properly logic from the
discovery-spi side).

Thoughts?



On Sat, Sep 29, 2018 at 11:43 PM Vyacheslav Daradur <[hidden email]> wrote:

>
> Hi Igniters!
>
> I think I found an illegal behavior in
> GridDhtPartitionsExchangeFuture#onDone, the following code is called
> here:
> ((DiscoveryCustomEvent)firstDiscoEvt).customMessage(null);
>
> That means a global instance of 'DiscoveryCustomEvent' is being
> mutated outside discovery-spi infrastructure. It also means that
> discovery listeners receive 'DiscoveryCustomEvent' with 'null' field
> instead of 'CustomMessage' which they may rely on.
>
> Could someone confirm if it is wrong behavior and should be fixed?
>
> --
> Best Regards, Vyacheslav D.

--
Best Regards, Vyacheslav D.

[Alexey Goncharuk]

Vyacheslav,

Thanks for investigating this. User code should never listen to system
custom events because this is an internal API and it's a subject to change.
If there is anything a user interested in, the corresponding public event
should be added.

Nullifying the event in this case looks ok for me.

вс, 30 сент. 2018 г. в 11:45, Vyacheslav Daradur <[hidden email]>:

> I think that I understand a reason for doing this:
> The most custom events which handle in
> 'GridDhtPartitionsExchangeFuture' are using only in PME flow and
> reason is release them for GC as soon as possible.
>
> But there are some other systems which can listen to the same events,
> for example, to perform activation/deactivation actions them should
> handle [ChangeGlobalStateMessage, ChangeGlobalStateFinishMessage]
> which can be reset to 'null' by PME earlier then they will be handled
> by other systems.
>
> I'd suggest do not reset to 'null' custom messages in
> 'DiscoveryCustomEvent ' (at least without properly logic from the
> discovery-spi side).
>
> Thoughts?
>
>
>
> On Sat, Sep 29, 2018 at 11:43 PM Vyacheslav Daradur <[hidden email]>
> wrote:
> >
> > Hi Igniters!
> >
> > I think I found an illegal behavior in
> > GridDhtPartitionsExchangeFuture#onDone, the following code is called
> > here:
> > ((DiscoveryCustomEvent)firstDiscoEvt).customMessage(null);
> >
> > That means a global instance of 'DiscoveryCustomEvent' is being
> > mutated outside discovery-spi infrastructure. It also means that
> > discovery listeners receive 'DiscoveryCustomEvent' with 'null' field
> > instead of 'CustomMessage' which they may rely on.
> >
> > Could someone confirm if it is wrong behavior and should be fixed?
> >
> > --
> > Best Regards, Vyacheslav D.
>
>
>
> --
> Best Regards, Vyacheslav D.
>

[daradurvs]

Alexey, thank you for the answer. I'd ask your advice about the
following question:

New Service Grid implementation listens to messages:
* ChangeGlobalStateMessage - to perform activation/deactivation actions;
* DynamicCacheChangeBatch - to handle caches stopping to undeploy
related affinity services;
* CacheAffinityChangeMessage - to recalculate assignments for affinity
services in case of late affinity.

It's important to handle these messages in order from disco-spi that
means SG is storing them in own exchange queue to process.
In some cases, PME may nullify this message earlier than they will be
processed by SG.

Could I exclude these messages from PME nullifying?

On Wed, Oct 3, 2018 at 11:26 AM Alexey Goncharuk
<[hidden email]> wrote:

>
> Vyacheslav,
>
> Thanks for investigating this. User code should never listen to system
> custom events because this is an internal API and it's a subject to change.
> If there is anything a user interested in, the corresponding public event
> should be added.
>
> Nullifying the event in this case looks ok for me.
>
> вс, 30 сент. 2018 г. в 11:45, Vyacheslav Daradur <[hidden email]>:
>
> > I think that I understand a reason for doing this:
> > The most custom events which handle in
> > 'GridDhtPartitionsExchangeFuture' are using only in PME flow and
> > reason is release them for GC as soon as possible.
> >
> > But there are some other systems which can listen to the same events,
> > for example, to perform activation/deactivation actions them should
> > handle [ChangeGlobalStateMessage, ChangeGlobalStateFinishMessage]
> > which can be reset to 'null' by PME earlier then they will be handled
> > by other systems.
> >
> > I'd suggest do not reset to 'null' custom messages in
> > 'DiscoveryCustomEvent ' (at least without properly logic from the
> > discovery-spi side).
> >
> > Thoughts?
> >
> >
> >
> > On Sat, Sep 29, 2018 at 11:43 PM Vyacheslav Daradur <[hidden email]>
> > wrote:
> > >
> > > Hi Igniters!
> > >
> > > I think I found an illegal behavior in
> > > GridDhtPartitionsExchangeFuture#onDone, the following code is called
> > > here:
> > > ((DiscoveryCustomEvent)firstDiscoEvt).customMessage(null);
> > >
> > > That means a global instance of 'DiscoveryCustomEvent' is being
> > > mutated outside discovery-spi infrastructure. It also means that
> > > discovery listeners receive 'DiscoveryCustomEvent' with 'null' field
> > > instead of 'CustomMessage' which they may rely on.
> > >
> > > Could someone confirm if it is wrong behavior and should be fixed?
> > >
> > > --
> > > Best Regards, Vyacheslav D.
> >
> >
> >
> > --
> > Best Regards, Vyacheslav D.
> >

--
Best Regards, Vyacheslav D.

[Alexey Goncharuk]

Vyacheslav,

I think it would be more correct to capture all required state that will be
further used in a custom object and use it later in service processor.
Nullifying the field is an explicit action that was taken to reduce memory
consumption on server nodes, so we cannot simply drop it. Another solution
is reference counting and nullifying the field only after all parties
finished processing the message, but it looks like an overengineering to me.

ср, 3 окт. 2018 г. в 15:00, Vyacheslav Daradur <[hidden email]>:

> Alexey, thank you for the answer. I'd ask your advice about the
> following question:
>
> New Service Grid implementation listens to messages:
> * ChangeGlobalStateMessage - to perform activation/deactivation actions;
> * DynamicCacheChangeBatch - to handle caches stopping to undeploy
> related affinity services;
> * CacheAffinityChangeMessage - to recalculate assignments for affinity
> services in case of late affinity.
>
> It's important to handle these messages in order from disco-spi that
> means SG is storing them in own exchange queue to process.
> In some cases, PME may nullify this message earlier than they will be
> processed by SG.
>
> Could I exclude these messages from PME nullifying?
>
> On Wed, Oct 3, 2018 at 11:26 AM Alexey Goncharuk
> <[hidden email]> wrote:
> >
> > Vyacheslav,
> >
> > Thanks for investigating this. User code should never listen to system
> > custom events because this is an internal API and it's a subject to
> change.
> > If there is anything a user interested in, the corresponding public event
> > should be added.
> >
> > Nullifying the event in this case looks ok for me.
> >
> > вс, 30 сент. 2018 г. в 11:45, Vyacheslav Daradur <[hidden email]>:
> >
> > > I think that I understand a reason for doing this:
> > > The most custom events which handle in
> > > 'GridDhtPartitionsExchangeFuture' are using only in PME flow and
> > > reason is release them for GC as soon as possible.
> > >
> > > But there are some other systems which can listen to the same events,
> > > for example, to perform activation/deactivation actions them should
> > > handle [ChangeGlobalStateMessage, ChangeGlobalStateFinishMessage]
> > > which can be reset to 'null' by PME earlier then they will be handled
> > > by other systems.
> > >
> > > I'd suggest do not reset to 'null' custom messages in
> > > 'DiscoveryCustomEvent ' (at least without properly logic from the
> > > discovery-spi side).
> > >
> > > Thoughts?
> > >
> > >
> > >
> > > On Sat, Sep 29, 2018 at 11:43 PM Vyacheslav Daradur <
> [hidden email]>
> > > wrote:
> > > >
> > > > Hi Igniters!
> > > >
> > > > I think I found an illegal behavior in
> > > > GridDhtPartitionsExchangeFuture#onDone, the following code is called
> > > > here:
> > > > ((DiscoveryCustomEvent)firstDiscoEvt).customMessage(null);
> > > >
> > > > That means a global instance of 'DiscoveryCustomEvent' is being
> > > > mutated outside discovery-spi infrastructure. It also means that
> > > > discovery listeners receive 'DiscoveryCustomEvent' with 'null' field
> > > > instead of 'CustomMessage' which they may rely on.
> > > >
> > > > Could someone confirm if it is wrong behavior and should be fixed?
> > > >
> > > > --
> > > > Best Regards, Vyacheslav D.
> > >
> > >
> > >
> > > --
> > > Best Regards, Vyacheslav D.
> > >
>
>
>
> --
> Best Regards, Vyacheslav D.
>

[daradurvs]

Alexey, thank you for the advice!


On Thu, Oct 4, 2018 at 1:25 PM Alexey Goncharuk
<[hidden email]> wrote:

>
> Vyacheslav,
>
> I think it would be more correct to capture all required state that will be
> further used in a custom object and use it later in service processor.
> Nullifying the field is an explicit action that was taken to reduce memory
> consumption on server nodes, so we cannot simply drop it. Another solution
> is reference counting and nullifying the field only after all parties
> finished processing the message, but it looks like an overengineering to me.
>
> ср, 3 окт. 2018 г. в 15:00, Vyacheslav Daradur <[hidden email]>:
>
> > Alexey, thank you for the answer. I'd ask your advice about the
> > following question:
> >
> > New Service Grid implementation listens to messages:
> > * ChangeGlobalStateMessage - to perform activation/deactivation actions;
> > * DynamicCacheChangeBatch - to handle caches stopping to undeploy
> > related affinity services;
> > * CacheAffinityChangeMessage - to recalculate assignments for affinity
> > services in case of late affinity.
> >
> > It's important to handle these messages in order from disco-spi that
> > means SG is storing them in own exchange queue to process.
> > In some cases, PME may nullify this message earlier than they will be
> > processed by SG.
> >
> > Could I exclude these messages from PME nullifying?
> >
> > On Wed, Oct 3, 2018 at 11:26 AM Alexey Goncharuk
> > <[hidden email]> wrote:
> > >
> > > Vyacheslav,
> > >
> > > Thanks for investigating this. User code should never listen to system
> > > custom events because this is an internal API and it's a subject to
> > change.
> > > If there is anything a user interested in, the corresponding public event
> > > should be added.
> > >
> > > Nullifying the event in this case looks ok for me.
> > >
> > > вс, 30 сент. 2018 г. в 11:45, Vyacheslav Daradur <[hidden email]>:
> > >
> > > > I think that I understand a reason for doing this:
> > > > The most custom events which handle in
> > > > 'GridDhtPartitionsExchangeFuture' are using only in PME flow and
> > > > reason is release them for GC as soon as possible.
> > > >
> > > > But there are some other systems which can listen to the same events,
> > > > for example, to perform activation/deactivation actions them should
> > > > handle [ChangeGlobalStateMessage, ChangeGlobalStateFinishMessage]
> > > > which can be reset to 'null' by PME earlier then they will be handled
> > > > by other systems.
> > > >
> > > > I'd suggest do not reset to 'null' custom messages in
> > > > 'DiscoveryCustomEvent ' (at least without properly logic from the
> > > > discovery-spi side).
> > > >
> > > > Thoughts?
> > > >
> > > >
> > > >
> > > > On Sat, Sep 29, 2018 at 11:43 PM Vyacheslav Daradur <
> > [hidden email]>
> > > > wrote:
> > > > >
> > > > > Hi Igniters!
> > > > >
> > > > > I think I found an illegal behavior in
> > > > > GridDhtPartitionsExchangeFuture#onDone, the following code is called
> > > > > here:
> > > > > ((DiscoveryCustomEvent)firstDiscoEvt).customMessage(null);
> > > > >
> > > > > That means a global instance of 'DiscoveryCustomEvent' is being
> > > > > mutated outside discovery-spi infrastructure. It also means that
> > > > > discovery listeners receive 'DiscoveryCustomEvent' with 'null' field
> > > > > instead of 'CustomMessage' which they may rely on.
> > > > >
> > > > > Could someone confirm if it is wrong behavior and should be fixed?
> > > > >
> > > > > --
> > > > > Best Regards, Vyacheslav D.
> > > >
> > > >
> > > >
> > > > --
> > > > Best Regards, Vyacheslav D.
> > > >
> >
> >
> >
> > --
> > Best Regards, Vyacheslav D.
> >

--
Best Regards, Vyacheslav D.