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

[Harsha Wardhana B]

Hi All,

Please review modified webrev located at,

http://cr.openjdk.java.net/~hb/8131061/webrev.02/

Regards

Harsha


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