diagnosticCommand.cpp:425 Should be "jdp.ttl".
On 6 dec 2012, at 13:49, Dmitry Samersoff <dmitry.samers...@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.samers...@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
