RR(M): 8002048.JDP v10

[Dmitry Samersoff]

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