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 <[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 > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * Give Rabbit time, and he'll always get the answer
