Static hierarchy in jmx tree

[¿¿¿¿¿ ¿¿¿¿¿¿¿¿¿]

Hello, everyone!

Ignite has a changeable jmx hierarchy now. It may lead a lot of problems for product maintenance and monitoring:
    - By default where would be classloader that change every time that node has been restarted. It will lead to rediscover all metrics for node.
    - It's hard to create a single template for different deployments. For example we should have about 4 different templates for each combination of classLoader and instanceName.
    - It's hard for engineers to switch between different hierarchies. You have to recreate anything you already have.

I've created https://issues.apache.org/jira/browse/IGNITE-12920 to change this behavior. I am going to:

    1. Change IGNITE_MBEAN_APPEND_CLASS_LOADER_ID default value to false
    2. Use instanceName in any case. If this option is set, the value will be used, otherwise use consistentId if it's null use nodeId
    3. Add option IGNITE_MBEAN_APPEND_INSTANCE_NAME for backward compatibility. True by default
    4. Update documentation according to changes

What do you think?

Igor

[¿¿¿¿¿ ¿¿¿¿¿¿¿¿¿]

Hello, everyone!
Because there was no cons, I have started developing and I have pull request https://github.com/apache/ignite/pull/8972 now.
Review it please.

> Sent: Friday, December 18, 2020 at 4:58 PM
> From: "¿¿¿¿¿ ¿¿¿¿¿¿¿¿¿" <[hidden email]>
> To: [hidden email]
> Subject: Static hierarchy in jmx tree
>
> Hello, everyone!
>
> Ignite has a changeable jmx hierarchy now. It may lead a lot of problems for product maintenance and monitoring:
>     - By default where would be classloader that change every time that node has been restarted. It will lead to rediscover all metrics for node.
>     - It's hard to create a single template for different deployments. For example we should have about 4 different templates for each combination of classLoader and instanceName.
>     - It's hard for engineers to switch between different hierarchies. You have to recreate anything you already have.
>
> I've created https://issues.apache.org/jira/browse/IGNITE-12920 to change this behavior. I am going to:
>
>     1. Change IGNITE_MBEAN_APPEND_CLASS_LOADER_ID default value to false
>     2. Use instanceName in any case. If this option is set, the value will be used, otherwise use consistentId if it's null use nodeId
>     3. Add option IGNITE_MBEAN_APPEND_INSTANCE_NAME for backward compatibility. True by default
>     4. Update documentation according to changes
>
> What do you think?
>
> Igor
>
>

[¿¿¿¿¿ ¿¿¿¿¿¿¿¿¿]

Is there anybody alive?

[Alexei Scherbakov]

Igor Akkuratov,

I have several concerns about your patch.

1. I don't understand why setting
IGNITE_MBEAN_APPEND_CLASS_LOADER_ID=false + igniteInstanceName is not a
solution to your issue ?
Just put it in the documentation and this should do fine.

2. Removing the classloader id can break the template working in the
container environment, where the instances with the same name are
instantiated using different classloaders.
How is this scenario supposed to work with a single template ?

3. Your patch introduces breaking change. This can be done only in two
steps: release N deprecated the behavior, release N + 1 changes the
behavior, according to the new rules.
But first let's decide if we really need to do any change at all.




<https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail>
Без
вирусов. www.avast.ru
<https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail>
<#m_-5956278403710209994_DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>

пт, 16 апр. 2021 г. в 14:16, Igor Akkuratov <[hidden email]>:

> Is there anybody alive?
>


--

Best regards,
Alexei Scherbakov

<https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail>
Без
вирусов. www.avast.ru
<https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail>
<#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>

[¿¿¿¿¿ ¿¿¿¿¿¿¿¿¿]

Alexei, thank you for your response.

> 1. I don't understand why setting
> IGNITE_MBEAN_APPEND_CLASS_LOADER_ID=false + igniteInstanceName is not a
> solution to your issue ?
> Just put it in the documentation and this should do fine.

It could be a solution for me, but I believe that such approach is a pain for a lot of users. Anyone who want to monitor Ignite node via JMX have to use both of options which have to be specified in different places.

> 2. Removing the classloader id can break the template working in the
> container environment, where the instances with the same name are
> instantiated using different classloaders.
> How is this scenario supposed to work with a single template ?

I am not sure that I got you. If we that is all about containers like docker, then there is no any problem here. Each application have to be started in separate container with there own network, so yes - jmx tree is the same, but there are from different hosts. So right now there are few scenarios:
- right now JMX is turned off by default, so there is no problem
- if you want to use jmx you have to turn it on manually and it would work. After my patch consistent id in case of persistent or node id in other cases would be used.
- if you want to specify instance name you can chose different names for different instances or separate them to different jvm.
- if you want to use the previous logic with class loader id, option MBEAN_APPEND_CLASS_LOADER_ID is still available.


> 3. Your patch introduces breaking change. This can be done only in two
> steps: release N deprecated the behavior, release N + 1 changes the
> behavior, according to the new rules.

Ok. If we take a decision, I will do that.

[Stanislav Lukyanov]

I guess the problem with this thread is that, on one hand, virtually anyone would support the change because no one likes the class loader IDs in JMX.
On the other hand, this is a breaking change and is kind of scary to do :)

> I am not sure that I got you. If we that is all about containers like docker, then there is no any problem here.

I believe Alexei means application containers, like WebSphere.

It seems to me that the entire IGNITE_MBEAN_APPEND_CLASS_LOADER_ID behavior solves one very narrow use case:
running multiple apps, each with an Ignite client, each with no instance name set; in this case it is indeed awkward to see that your app with instance name = null can't start because there is already a different app with instance name = null.

How about the following behavior?
- Always use EITHER instance name or class loader ID
- If the node doesn't have an instance name set, use its class loader ID
-- This way multiple client nodes in multiple apps in the same container can work together
- If the node does have an instance name set, use it without class loader ID
-- This way people with instance names set will get nice stable JMX hierarchies
- In both cases, the ObjectName will be
-- org.apache.ignite.<XYZ>.<METRICS> where <XYZ> is either the instance name of the class loader ID. In both cases, the metrics are on the same nesting level which is good I guess..

Since this is not a core API I think it's alright to break the behavior in one minor release - given that there is a fallback option.
With that in mind, I'd rather have a new option like IGNITE_MBEAN_STATIC_HIERARCHY
- IGNITE_MBEAN_STATIC_HIERARCHY=true (default): the new behavior is in effect, IGNITE_MBEAN_APPEND_CLASS_LOADER_ID is ignored
- IGNITE_MBEAN_STATIC_HIERARCHY=false: the old behavior is in effect

Thanks,
Stan

> On 10 May 2021, at 23:04, Igor Akkuratov <[hidden email]> wrote:
>
> Alexei, thank you for your response.
>
>> 1. I don't understand why setting
>> IGNITE_MBEAN_APPEND_CLASS_LOADER_ID=false + igniteInstanceName is not a
>> solution to your issue ?
>> Just put it in the documentation and this should do fine.
>
> It could be a solution for me, but I believe that such approach is a pain for a lot of users. Anyone who want to monitor Ignite node via JMX have to use both of options which have to be specified in different places.
>
>> 2. Removing the classloader id can break the template working in the
>> container environment, where the instances with the same name are
>> instantiated using different classloaders.
>> How is this scenario supposed to work with a single template ?
>
> I am not sure that I got you. If we that is all about containers like docker, then there is no any problem here. Each application have to be started in separate container with there own network, so yes - jmx tree is the same, but there are from different hosts. So right now there are few scenarios:
> - right now JMX is turned off by default, so there is no problem
> - if you want to use jmx you have to turn it on manually and it would work. After my patch consistent id in case of persistent or node id in other cases would be used.
> - if you want to specify instance name you can chose different names for different instances or separate them to different jvm.
> - if you want to use the previous logic with class loader id, option MBEAN_APPEND_CLASS_LOADER_ID is still available.
>
>
>> 3. Your patch introduces breaking change. This can be done only in two
>> steps: release N deprecated the behavior, release N + 1 changes the
>> behavior, according to the new rules.
>
> Ok. If we take a decision, I will do that.

[¿¿¿¿¿ ¿¿¿¿¿¿¿¿¿]

Stanislav, thank you for your response.
I am fine with the option IGNITE_MBEAN_STATIC_HIERARCHY=true, is looks fair to give the opportunity to use the old behavior. But I do not understand why there is a classloader, I don't know any reason for that, whenever there is already a unique id - node id. It looks much more appropriate for me to use node id instead of classloader. So my idea is to use either instance name or consistentId or  nodeId is there any cons why it's better to do that your way?

> Sent: Monday, June 07, 2021 at 4:21 PM
> From: "Stanislav Lukyanov" <[hidden email]>
> To: [hidden email]
> Subject: Re: Static hierarchy in jmx tree
>
> I guess the problem with this thread is that, on one hand, virtually anyone would support the change because no one likes the class loader IDs in JMX.
> On the other hand, this is a breaking change and is kind of scary to do :)
>
> > I am not sure that I got you. If we that is all about containers like docker, then there is no any problem here.
>
> I believe Alexei means application containers, like WebSphere.
>
> It seems to me that the entire IGNITE_MBEAN_APPEND_CLASS_LOADER_ID behavior solves one very narrow use case:
> running multiple apps, each with an Ignite client, each with no instance name set; in this case it is indeed awkward to see that your app with instance name = null can't start because there is already a different app with instance name = null.
>
> How about the following behavior?
> - Always use EITHER instance name or class loader ID
> - If the node doesn't have an instance name set, use its class loader ID
> -- This way multiple client nodes in multiple apps in the same container can work together
> - If the node does have an instance name set, use it without class loader ID
> -- This way people with instance names set will get nice stable JMX hierarchies
> - In both cases, the ObjectName will be
> -- org.apache.ignite.<XYZ>.<METRICS> where <XYZ> is either the instance name of the class loader ID. In both cases, the metrics are on the same nesting level which is good I guess..
>
> Since this is not a core API I think it's alright to break the behavior in one minor release - given that there is a fallback option.
> With that in mind, I'd rather have a new option like IGNITE_MBEAN_STATIC_HIERARCHY
> - IGNITE_MBEAN_STATIC_HIERARCHY=true (default): the new behavior is in effect, IGNITE_MBEAN_APPEND_CLASS_LOADER_ID is ignored
> - IGNITE_MBEAN_STATIC_HIERARCHY=false: the old behavior is in effect
>
> Thanks,
> Stan
>
> > On 10 May 2021, at 23:04, Igor Akkuratov <[hidden email]> wrote:
> >
> > Alexei, thank you for your response.
> >
> >> 1. I don't understand why setting
> >> IGNITE_MBEAN_APPEND_CLASS_LOADER_ID=false + igniteInstanceName is not a
> >> solution to your issue ?
> >> Just put it in the documentation and this should do fine.
> >
> > It could be a solution for me, but I believe that such approach is a pain for a lot of users. Anyone who want to monitor Ignite node via JMX have to use both of options which have to be specified in different places.
> >
> >> 2. Removing the classloader id can break the template working in the
> >> container environment, where the instances with the same name are
> >> instantiated using different classloaders.
> >> How is this scenario supposed to work with a single template ?
> >
> > I am not sure that I got you. If we that is all about containers like docker, then there is no any problem here. Each application have to be started in separate container with there own network, so yes - jmx tree is the same, but there are from different hosts. So right now there are few scenarios:
> > - right now JMX is turned off by default, so there is no problem
> > - if you want to use jmx you have to turn it on manually and it would work. After my patch consistent id in case of persistent or node id in other cases would be used.
> > - if you want to specify instance name you can chose different names for different instances or separate them to different jvm.
> > - if you want to use the previous logic with class loader id, option MBEAN_APPEND_CLASS_LOADER_ID is still available.
> >
> >
> >> 3. Your patch introduces breaking change. This can be done only in two
> >> steps: release N deprecated the behavior, release N + 1 changes the
> >> behavior, according to the new rules.
> >
> > Ok. If we take a decision, I will do that.
>
>