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.fu...@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.fu...@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

Reply via email to