RR(M): 8002048.JDP v11

[Staffan Larsen]

diagnosticCommand.cpp:425
Should be "jdp.ttl".


On 6 dec 2012, at 13:49, Dmitry Samersoff <Dmitry.Samersoff at oracle.com> wrote:

> Staffan,
> 
> Addressed you concerns,
> 
> Changed test
> 
> http://washi.ru.oracle.com/ds160299/webrev/8002048.JDP/webrev.11/
> 
> Could you review a CCC request?
> 
> -Dmitry
> 
> On 2012-12-04 19:04, Staffan Larsen wrote:
>> 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
>> 
> 
> 
> -- 
> Dmitry Samersoff
> Oracle Java development team, Saint Petersburg, Russia
> * Give Rabbit time, and he'll always get the answer