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

[Harsha Wardhana B]

Thanks for the review Daniel.

-Harsha


On Thursday 25 August 2016 02:15 PM, Daniel Fuchs wrote:
> 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
>>
>