Staffan, Thank you for detailed review, see below inline.
On 2012-12-04 19:04, Staffan Larsen wrote: > 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. >From IPv4 perspective port could be a service name and I hope that Java will have getservicebyname() API shortly. But I can change it. > Agent.java: startDiscoveryService Indentation seems flawed. OK. > > 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". There are no such things as public broadcast address or public broadcast port. If we decide to set some default we should ask IANA to assign one for Java/JDP. > Agent.java: 279 Pass the UnknownHostException as the cause parameter > to AgentConfigurationError. OK. > Agent.java: 286 A better error message would be "Could not parse JDP > port argument: "+discoveryPort Agree with "Couldn't parse". We should have a generic policy whether we quote user arguments in exceptions or not. I'm paranoid and prefer don't quote. But can change it (in all places). > 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. These properties moved to JDPController intentionally - average user shouldn't change it, as it can cause security or network problem. i.e. properties below could be set either in config file or in command line: com.sun.management.jdp.port com.sun.management.jdp.address com.sun.management.jdp.name properties below have to be set explicitly using command line. com.sun.management.jdp.ttl com.sun.management.jdp.pause com.sun.management.jdp.source_addr I'll document it. > JDPController.java: 150 sun.java.command can be empty or null which > you need to handle. OK. > > JDPController.java: 161 A better name for the thread could be "JDP > Broadcaster". OK. > > JDPController.java: 136 You set the default pause time to 60 ms. Is > that intentional? It seems very small. Will fix it. > > 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? Agree with IOException. I have no ideas how to notify user that something bad happens with broadcast socket as broadcaster run in background thread. > 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. stopDiscoveryService have to be called from stopAgent - my bad will fix it. > JDPBroadcaster.java Don't you need to set the SO_BROADCAST option on > the DatagramChannel? We shouldn't. But I'll re-check it. > 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? It should be 1 as in all previous webrevs - left from my internal tests. > JDPBroadcaster.java: 102 This constructor is never used. Should we > remove it? It's a convenience interface for developers/SQE who would like to use broadcaster directly, because setting source address for broadcast is very rare case. I think it's worth to leave it. > JDPException.java Why is this a RuntimeException? Shouldn't it be a > checked exception? I'll fix it. > JDPJMXPacket The class has an equals() method but no hashCode(). OK. I'll add it. > JDPJMXPacket:172 nit: en extra space OK. > JDPJMXPacket:168-170 nit: add space after the comma OK. > > Indentation seems flawed for all new files. OK. > 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? We discussed it with Marcus Hirt here in SPB, and come to agreement to continue without these two fields for now but file a bug for it (done). I don't like a minute-to-train changes, also value of these two filed is doubtful (we don't have reliable getPID() on all platforms, broadcast period doesn't correlate with actual packet rate). -Dmitry > > 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
