[DISCUSS] ThreadGroup for IgniteThread

[Ivan Pavlukhin]

Hi,

As you might know, IgniteThread class captures calling ThreadGroup on
initialization (as IgniteThread.DFLT_GRP) and includes all new ignite
threads into this group. A user reported an issue [1] related to it.
And the root cause here is that captured DFLT_GRP is out of control of
IgniteThread class. Looks like a design fault. Consequently several
unclear points:
1. What is the real need for IgniteThread.DFLT_GRP?
2. Can we simply stop using this trick?
3. Could there be any better options to do the same?

Please share your thoughts.

[1] https://issues.apache.org/jira/browse/IGNITE-12554

--
Best regards,
Ivan Pavlukhin

[Alexey Goncharuk]

Ivan,

I cannot recall why exactly a separate thread group was needed. I guess the
intention was to collect all threads related to Ignite to one group, but I
see no practical use of that particular implementation.

We can either remove it (not sure if this is a breaking public API change?)
or create a separate thread group per Ignite instance and pass it to the
constructor of IgniteThread (quite a lot of refactoring).

вт, 21 янв. 2020 г. в 13:17, Ivan Pavlukhin <[hidden email]>:

> Hi,
>
> As you might know, IgniteThread class captures calling ThreadGroup on
> initialization (as IgniteThread.DFLT_GRP) and includes all new ignite
> threads into this group. A user reported an issue [1] related to it.
> And the root cause here is that captured DFLT_GRP is out of control of
> IgniteThread class. Looks like a design fault. Consequently several
> unclear points:
> 1. What is the real need for IgniteThread.DFLT_GRP?
> 2. Can we simply stop using this trick?
> 3. Could there be any better options to do the same?
>
> Please share your thoughts.
>
> [1] https://issues.apache.org/jira/browse/IGNITE-12554
>
> --
> Best regards,
> Ivan Pavlukhin
>

[Ivan Pavlukhin]

Alex,

> We can either remove it (not sure if this is a breaking public API change?)
> or create a separate thread group per Ignite instance and pass it to the
> constructor of IgniteThread (quite a lot of refactoring).

Recently there were a discussion about "magic stuff" in codebase. And
it seems that we should eliminate such stuff if there is no chance to
understand why is it needed.

I run TC after dropping special ThreadGroup and did not get any new
failures [1]. It can imagine that dedicated ThreadGroup has some sense
for application servers. But personally I would prefer to get rid of
that ThreadGroup. A more conservative approach is to use some flag
(system property) to control it.

[1] https://mtcga.gridgain.com/pr.html?serverId=apache&suiteId=IgniteTests24Java8_RunAll&branchForTc=pull/7297/head&action=Latest

вт, 21 янв. 2020 г. в 16:39, Alexey Goncharuk <[hidden email]>:

>
> Ivan,
>
> I cannot recall why exactly a separate thread group was needed. I guess the
> intention was to collect all threads related to Ignite to one group, but I
> see no practical use of that particular implementation.
>
> We can either remove it (not sure if this is a breaking public API change?)
> or create a separate thread group per Ignite instance and pass it to the
> constructor of IgniteThread (quite a lot of refactoring).
>
> вт, 21 янв. 2020 г. в 13:17, Ivan Pavlukhin <[hidden email]>:
>
> > Hi,
> >
> > As you might know, IgniteThread class captures calling ThreadGroup on
> > initialization (as IgniteThread.DFLT_GRP) and includes all new ignite
> > threads into this group. A user reported an issue [1] related to it.
> > And the root cause here is that captured DFLT_GRP is out of control of
> > IgniteThread class. Looks like a design fault. Consequently several
> > unclear points:
> > 1. What is the real need for IgniteThread.DFLT_GRP?
> > 2. Can we simply stop using this trick?
> > 3. Could there be any better options to do the same?
> >
> > Please share your thoughts.
> >
> > [1] https://issues.apache.org/jira/browse/IGNITE-12554
> >
> > --
> > Best regards,
> > Ivan Pavlukhin
> >

--
Best regards,
Ivan Pavlukhin

[Alexey Goncharuk]

Ivan,

I believe that the removal of the thread group is harmless. Let's check
with the rest of the community, and if there is no objections, remove it.

пт, 24 янв. 2020 г. в 11:13, Ivan Pavlukhin <[hidden email]>:

> Alex,
>
> > We can either remove it (not sure if this is a breaking public API
> change?)
> > or create a separate thread group per Ignite instance and pass it to the
> > constructor of IgniteThread (quite a lot of refactoring).
>
> Recently there were a discussion about "magic stuff" in codebase. And
> it seems that we should eliminate such stuff if there is no chance to
> understand why is it needed.
>
> I run TC after dropping special ThreadGroup and did not get any new
> failures [1]. It can imagine that dedicated ThreadGroup has some sense
> for application servers. But personally I would prefer to get rid of
> that ThreadGroup. A more conservative approach is to use some flag
> (system property) to control it.
>
> [1]
> https://mtcga.gridgain.com/pr.html?serverId=apache&suiteId=IgniteTests24Java8_RunAll&branchForTc=pull/7297/head&action=Latest
>
> вт, 21 янв. 2020 г. в 16:39, Alexey Goncharuk <[hidden email]
> >:
> >
> > Ivan,
> >
> > I cannot recall why exactly a separate thread group was needed. I guess
> the
> > intention was to collect all threads related to Ignite to one group, but
> I
> > see no practical use of that particular implementation.
> >
> > We can either remove it (not sure if this is a breaking public API
> change?)
> > or create a separate thread group per Ignite instance and pass it to the
> > constructor of IgniteThread (quite a lot of refactoring).
> >
> > вт, 21 янв. 2020 г. в 13:17, Ivan Pavlukhin <[hidden email]>:
> >
> > > Hi,
> > >
> > > As you might know, IgniteThread class captures calling ThreadGroup on
> > > initialization (as IgniteThread.DFLT_GRP) and includes all new ignite
> > > threads into this group. A user reported an issue [1] related to it.
> > > And the root cause here is that captured DFLT_GRP is out of control of
> > > IgniteThread class. Looks like a design fault. Consequently several
> > > unclear points:
> > > 1. What is the real need for IgniteThread.DFLT_GRP?
> > > 2. Can we simply stop using this trick?
> > > 3. Could there be any better options to do the same?
> > >
> > > Please share your thoughts.
> > >
> > > [1] https://issues.apache.org/jira/browse/IGNITE-12554
> > >
> > > --
> > > Best regards,
> > > Ivan Pavlukhin
> > >
>
>
>
> --
> Best regards,
> Ivan Pavlukhin
>

[Ivan Pavlukhin]

Igniters,

I prepared PR removing custom ThreadGroup for a ticket [1]. Everybody
is welcome to review. If there will be no objections I am going to
merge the patch by the end of this week.

[1] https://issues.apache.org/jira/browse/IGNITE-12554

пт, 24 янв. 2020 г. в 13:15, Alexey Goncharuk <[hidden email]>:

>
> Ivan,
>
> I believe that the removal of the thread group is harmless. Let's check
> with the rest of the community, and if there is no objections, remove it.
>
> пт, 24 янв. 2020 г. в 11:13, Ivan Pavlukhin <[hidden email]>:
>
> > Alex,
> >
> > > We can either remove it (not sure if this is a breaking public API
> > change?)
> > > or create a separate thread group per Ignite instance and pass it to the
> > > constructor of IgniteThread (quite a lot of refactoring).
> >
> > Recently there were a discussion about "magic stuff" in codebase. And
> > it seems that we should eliminate such stuff if there is no chance to
> > understand why is it needed.
> >
> > I run TC after dropping special ThreadGroup and did not get any new
> > failures [1]. It can imagine that dedicated ThreadGroup has some sense
> > for application servers. But personally I would prefer to get rid of
> > that ThreadGroup. A more conservative approach is to use some flag
> > (system property) to control it.
> >
> > [1]
> > https://mtcga.gridgain.com/pr.html?serverId=apache&suiteId=IgniteTests24Java8_RunAll&branchForTc=pull/7297/head&action=Latest
> >
> > вт, 21 янв. 2020 г. в 16:39, Alexey Goncharuk <[hidden email]
> > >:
> > >
> > > Ivan,
> > >
> > > I cannot recall why exactly a separate thread group was needed. I guess
> > the
> > > intention was to collect all threads related to Ignite to one group, but
> > I
> > > see no practical use of that particular implementation.
> > >
> > > We can either remove it (not sure if this is a breaking public API
> > change?)
> > > or create a separate thread group per Ignite instance and pass it to the
> > > constructor of IgniteThread (quite a lot of refactoring).
> > >
> > > вт, 21 янв. 2020 г. в 13:17, Ivan Pavlukhin <[hidden email]>:
> > >
> > > > Hi,
> > > >
> > > > As you might know, IgniteThread class captures calling ThreadGroup on
> > > > initialization (as IgniteThread.DFLT_GRP) and includes all new ignite
> > > > threads into this group. A user reported an issue [1] related to it.
> > > > And the root cause here is that captured DFLT_GRP is out of control of
> > > > IgniteThread class. Looks like a design fault. Consequently several
> > > > unclear points:
> > > > 1. What is the real need for IgniteThread.DFLT_GRP?
> > > > 2. Can we simply stop using this trick?
> > > > 3. Could there be any better options to do the same?
> > > >
> > > > Please share your thoughts.
> > > >
> > > > [1] https://issues.apache.org/jira/browse/IGNITE-12554
> > > >
> > > > --
> > > > Best regards,
> > > > Ivan Pavlukhin
> > > >
> >
> >
> >
> > --
> > Best regards,
> > Ivan Pavlukhin
> >

--
Best regards,
Ivan Pavlukhin

[Ivan Pavlukhin]

I merged PR and resolved the issue [1].

[1] https://issues.apache.org/jira/browse/IGNITE-12554

вт, 28 янв. 2020 г. в 08:45, Ivan Pavlukhin <[hidden email]>:

>
> Igniters,
>
> I prepared PR removing custom ThreadGroup for a ticket [1]. Everybody
> is welcome to review. If there will be no objections I am going to
> merge the patch by the end of this week.
>
> [1] https://issues.apache.org/jira/browse/IGNITE-12554
>
> пт, 24 янв. 2020 г. в 13:15, Alexey Goncharuk <[hidden email]>:
> >
> > Ivan,
> >
> > I believe that the removal of the thread group is harmless. Let's check
> > with the rest of the community, and if there is no objections, remove it.
> >
> > пт, 24 янв. 2020 г. в 11:13, Ivan Pavlukhin <[hidden email]>:
> >
> > > Alex,
> > >
> > > > We can either remove it (not sure if this is a breaking public API
> > > change?)
> > > > or create a separate thread group per Ignite instance and pass it to the
> > > > constructor of IgniteThread (quite a lot of refactoring).
> > >
> > > Recently there were a discussion about "magic stuff" in codebase. And
> > > it seems that we should eliminate such stuff if there is no chance to
> > > understand why is it needed.
> > >
> > > I run TC after dropping special ThreadGroup and did not get any new
> > > failures [1]. It can imagine that dedicated ThreadGroup has some sense
> > > for application servers. But personally I would prefer to get rid of
> > > that ThreadGroup. A more conservative approach is to use some flag
> > > (system property) to control it.
> > >
> > > [1]
> > > https://mtcga.gridgain.com/pr.html?serverId=apache&suiteId=IgniteTests24Java8_RunAll&branchForTc=pull/7297/head&action=Latest
> > >
> > > вт, 21 янв. 2020 г. в 16:39, Alexey Goncharuk <[hidden email]
> > > >:
> > > >
> > > > Ivan,
> > > >
> > > > I cannot recall why exactly a separate thread group was needed. I guess
> > > the
> > > > intention was to collect all threads related to Ignite to one group, but
> > > I
> > > > see no practical use of that particular implementation.
> > > >
> > > > We can either remove it (not sure if this is a breaking public API
> > > change?)
> > > > or create a separate thread group per Ignite instance and pass it to the
> > > > constructor of IgniteThread (quite a lot of refactoring).
> > > >
> > > > вт, 21 янв. 2020 г. в 13:17, Ivan Pavlukhin <[hidden email]>:
> > > >
> > > > > Hi,
> > > > >
> > > > > As you might know, IgniteThread class captures calling ThreadGroup on
> > > > > initialization (as IgniteThread.DFLT_GRP) and includes all new ignite
> > > > > threads into this group. A user reported an issue [1] related to it.
> > > > > And the root cause here is that captured DFLT_GRP is out of control of
> > > > > IgniteThread class. Looks like a design fault. Consequently several
> > > > > unclear points:
> > > > > 1. What is the real need for IgniteThread.DFLT_GRP?
> > > > > 2. Can we simply stop using this trick?
> > > > > 3. Could there be any better options to do the same?
> > > > >
> > > > > Please share your thoughts.
> > > > >
> > > > > [1] https://issues.apache.org/jira/browse/IGNITE-12554
> > > > >
> > > > > --
> > > > > Best regards,
> > > > > Ivan Pavlukhin
> > > > >
> > >
> > >
> > >
> > > --
> > > Best regards,
> > > Ivan Pavlukhin
> > >
>
>
>
> --
> Best regards,
> Ivan Pavlukhin

--
Best regards,
Ivan Pavlukhin