RFR : 8131061 - Use of -Dcom.sun.management.snmp needs to be examined for modules

[Harsha Wardhana B]

On Wednesday 24 August 2016 01:10 AM, Mandy Chung wrote:
>> On Aug 23, 2016, at 8:21 AM, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:
>>
>> On 23/08/16 15:45, Harsha Wardhana B wrote:
>>> Hi Daniel,
>>>
>>> The focus of this issue was to decouple hard-dependency between
>>> java.management module and jdk.snmp module since jdk.snmp is not a core
>>> module. There was not much focus on an 'Agent' Interface that could
>>> allow plugging in any Agent. Hence there was no discussions around the
>>> structure of such an interface.
>>>
>> OK - sun/management/spi/AgentProvider.java is a private
>> interface that only classes in the JDK can implement, and
>> is not designed to be extended outside of the JDK, so I
>> agree that what you have seems to be sufficient for the purpose
>> of decoupling. Because nothing outside of the JDK can implement
>> it then it can be revisited and amended later if we have other uses.
>>
> Correct this is a private interface that can be changed in any future release.  This work allows the SNMP management agent be loaded via service binding rather than requiring the module be explicitly added to the module graph.
>
> Daniel is closer to the snmp agent implementation and I agree that AgentProvider can be simplified.  See my comment inlined below.
>
>> On Aug 23, 2016, at 3:41 AM, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:
>>
>> On 22/08/16 22:58, Mandy Chung wrote:
>>> sun/management/spi/AgentProvider.java
>>>   78     public abstract void startAgent(String port, Properties props);
>>>
>>> The port parameter should be “int”.
>>>
>> I wonder about that. I wonder if the name of the agent provider
>> should be the name of the property that starts it - for
>> instance - we could have an agent provider whose name is “com.sun.management.snmp.port"
> “com.sun.management.snmp.port” be the provider name seems confusing (it’s an agent but not a port).  I considered naming it with “com.sun.management.snmp.Agent” but since the name is private interface, a simple name would work.
>
>> - and the first parameter to
>> startAgent would be the value associated with that
>> property (we'd renamed the port parameter into e.g.
>> propertyValue).
>>
>> Then it would be up to the agent provider to interpret the
>> property value however it sees fit. In this example - a
>> provider deployed as responding to the "com.sun.management.snmp.port"
>> property would interpret the value of the property as the SNMP
>> port number of the SNMP agent to start.
>>
>> Our implementation could still only look for an agent
>> provider named "com.sun.management.snmp.port" (instead of
>> "SnmpAgent") - but that could be extended in the future
>> if we ever want to start more (different) agents.
>>
> While it’s easy to rev the interface multiple agents, we should wait until it’s really needed.
>
> I wasn’t aware of any customer using SNMP management support.  IMO we should consider if SNMP support should be deprecated for removal.  So I would keep the change minimal.
>
>> Also I'm not sure the AgentProvider should have a getPort() method.
>> I don't see were it is used? Is it for debugging purposes?
>> If so maybe it should be getAddress() and return a informal String.
>>
> Good point - it should be an address rather than a port number.  If it’s not used, let’s drop it.  I also suggest to drop the “port” parameter and the snmp agent can get the “com.sun.management.snmp.port” property from the specified props.
>
>     public abstract void startAgent(Properties props);
>
> Harsha - are you okay with that?
Yes. I am fine with it.
>
> Mandy
Harsha