RR(M): 8002048.JDP v10
Dmitry Samersoff
dmitry.samersoff at oracle.com
Tue Dec 4 08:32:56 PST 2012
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
> <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
More information about the serviceability-dev
mailing list