<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Hi Alessio, <br>
<br>
Thanks for the review! Please see my comments inline. <br>
<br>
On 08/07/14 18:45, Alessio Soldano wrote:<br>
</div>
<blockquote cite="mid:53BBCBC8.5050505@redhat.com" type="cite"> ResponseTime(*)Interceptor
<br>
--------------------------------------
<br>
* The AbstractResponseTimeInterceptor#getServiceCounterName method
will be using the counter name we set in the
EndpointAssociationInterceptor, good. However, from a CXF point of
view, the name could be computed each time, so the code doing that
needs to be decently optimized. In particular, the StringBuilder
should be properly used there; if there's a StringBuilder being
used, you should really avoid string concatenations to add stuff
in the builder. The JDK optimizer will replace the string
concatenations with additional StringBuilder instances, which is a
waste of resources. So basically use multiple append() invocations
instead of String concatenations (unless the String constants only
are being concatenated).
<br>
</blockquote>
These are all legacy codes from cxf, and another 5 StringBuilder
instance will be created for the string concatenations. But it is
more readable than all with append(). Should we pay the additional
resources to get <span class="keyword" style="margin-right: 15px;
vertical-align: bottom;">readability?</span>
<meta http-equiv="content-type" content="text/html;
charset=ISO-8859-1">
<blockquote cite="mid:53BBCBC8.5050505@redhat.com" type="cite">*
same reasoning for
AbstractResponseTimeInterceptor#getOperationCounterName, plus
please use StringBuilder instead of StringBuffer, as that's not
accessed concurrently.
<br>
</blockquote>
I wrote this wrong. It's a local variable and doesn't have some
concurrent issue,so StringBuilder will be fine, no need StringBuff.<br>
<blockquote cite="mid:53BBCBC8.5050505@redhat.com" type="cite">* In
all ResponseTimeMessage(*)Interceptor, the check on
"forceDisabled" should come before the isServiceCounterEnabled()
invocation, as that's expensive. So basically do "if
!forceDisabled && (isServiceCounterEnabled(ex))" instead
of "if (isServiceCounterEnabled(ex) && !forceDisabled)"
<br>
</blockquote>
I'll make this change.<br>
<blockquote cite="mid:53BBCBC8.5050505@redhat.com" type="cite">
<br>
ResponseTimeCounter
<br>
-------------------------------
<br>
* Generally speaking I have some concerns on the thread safety of
this class, in particular on visibility. Shouldn't we have final
or volatile attributes?
<br>
</blockquote>
Looked at this class again. The totalHandlingTime, minHandlingTime
and maxHandlingTime should be all atomic variable. <br>
<blockquote cite="mid:53BBCBC8.5050505@redhat.com" type="cite">* At
line 87, did you really want to use "|" or "||" ?
<br>
</blockquote>
typo !<br>
<blockquote cite="mid:53BBCBC8.5050505@redhat.com" type="cite">* Is
it possible to end up with handlingTime = 0 for oneway messages
and hence end up setting min handling time to 0 ? Is that
reasonable ? (probably no...)
<br>
</blockquote>
no. The oneway message will go into several interceptors and
consume some time to finish the process.<br>
<blockquote cite="mid:53BBCBC8.5050505@redhat.com" type="cite">* the
update of min and max handling time in not performed in a thread
safe way; please have a look at how that should possible be done
in our former EndpointMetricImpl (see updateMin / updateMax in
[1])
<br>
</blockquote>
It should be all atomic variable here.<br>
<blockquote cite="mid:53BBCBC8.5050505@redhat.com" type="cite">* it
is possible to get unreliable values for the average response time
in highly concurrent environments; we might decide to accept this
or solve as we did in our EndpointMetricsImpl (see the usage or
read/write lock and related comment in [1])
<br>
</blockquote>
I am not sure if the lock is a bit heavy here. It is possible to
affect the interceptor execution performance , and it will wait the
lock, then the last interceptor MessageSenderEndingInterceptor can
really send the response to the client.Do you <br>
think compareAndSet() can help solve this problem ?<br>
<br>
<blockquote cite="mid:53BBCBC8.5050505@redhat.com" type="cite">
<br>
EndpointAssociationInterceptor
<br>
-------------------------------------------
<br>
* We should probably set the service counter name in the exchange
only when the statistics are enabled.
<br>
</blockquote>
I'll refactor this.<br>
<blockquote cite="mid:53BBCBC8.5050505@redhat.com" type="cite">
<br>
EndpointMetricsCXFAdapterImpl
<br>
--------------------------------------------
<br>
* counterRepo and counter members should be final, while there's
no need for keeping a reference to the objectName
<br>
</blockquote>
Make it as method local final variable will be better. <br>
<br>
<br>
Cheers,<br>
Jim<br>
<br>
<br>
</body>
</html>