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

[Daniel Fuchs]

On 25/08/16 07:40, Harsha Wardhana B wrote:
> Hi All,
>
> Please review modified webrev located at,
>
> http://cr.openjdk.java.net/~hb/8131061/webrev.02/
>
> Regards
>
> Harsha

Hi Harsha,

Looks good to me!

best regards,

-- daniel

>
>
> On Wednesday 24 August 2016 11:28 AM, Harsha Wardhana B wrote:
>>
>>
>> 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
>