RR(M): 8002048.JDP v10

[Staffan Larsen]

Hi Dmitry,

diagnosticCommand.cpp:414 / diagnosticCommand.hpp:238:
Shouldn't the type of the port argument be INTEGER? Although I can't find any information on what the possible types are.

Agent.java: startDiscoveryService
Indentation seems flawed.

Why is there no default values for JDP address and port? Having default values would make it much easier to use. As I understand the code you would now have to say "-Dcom.sun.management.jdp.port=4711 -Dcom.sun.management.jdp.address=172.31.255.255". It would be simpler with something like "-Dcom.sun.management.jdp.enable".

Agent.java: 279
Pass the UnknownHostException as the cause parameter to AgentConfigurationError.

Agent.java: 286
A better error message would be "Could not parse JDP port argument: "+discoveryPort

JDPController.java: startDiscoveryService
The properties that are read here are read from System.getProperty, but the properties read in Agent.java (com.sun.management.jdp.port and com.sun.management.jdp.address) can come from either System.getProperty or the file specified with com.sun.management.config.file. Why this difference? Shouldn't all properties be specified in the same locations? "com.sun.management.jdp.name" is also read from System properties in Agent.java.

JDPController.java: 150
sun.java.command can be empty or null which you need to handle.

JDPController.java: 161
A better name for the thread could be "JDP Broadcaster".

JDPController.java: 136
You set the default pause time to 60 ms. Is that intentional? It seems very small.

JDPController.java: 72
Catching the IOException outside the while loop would simplify the code. Should we notify the user somehow if this exception happens and we shut down the broadcaster?

JDPController.java: 169
There are no calls to stopDiscoveryService(). Should we include code that is never called? Is it tested that it works? If we plan to add calls to it in the future, I would prefer to delay the introduction of the code until it is used.

JDPBroadcaster.java
Don't you need to set the SO_BROADCAST option on the DatagramChannel?

You set the IP_MULTICAST_TTL to 0 by default. According to the javadoc for DatagramChannel on IPv4: "Datagrams with a TTL of zero are not transmitted on the network but may be delivered locally." Is this intentional?

JDPBroadcaster.java: 102
This constructor is never used. Should we remove it?

JDPException.java
Why is this a RuntimeException? Shouldn't it be a checked exception?

JDPJMXPacket
The class has an equals() method but no hashCode().

JDPJMXPacket:172
nit: en extra space

JDPJMXPacket:168-170
nit: add space after the comma

Indentation seems flawed for all new files.

I have not (yet) looked at the tests or the javadoc.

There were discussions about adding the OS process identifier and the broadcast period as fields in the packets. What happened to these features?

Thanks,
/Staffan



On 3 dec 2012, at 23:35, Dmitry Samersoff <Dmitry.Samersoff at oracle.com> wrote:

> Hi Everybody,
> 
> I hope it's a final round of JDP
> 
> http://washi.ru.oracle.com/ds160299/webrev/8002048.JDP/webrev.10/
> 
> 1. Some bugs and nits fixed
> 
> 2. Improved javadoc comments (generated javadoc resides in the same
> directory, all comments is much appreciated as English is not my native
> language)
> 
> 3. Significantly changed tests
> 
> -Dmitry
> 
> -- 
> Dmitry Samersoff
> Oracle Java development team, Saint Petersburg, Russia
> * Give Rabbit time, and he'll always get the answer