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 <[email protected]> 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
