Hi Alessio,
Thanks for the review! Please see my comments inline.
On 08/07/14 18:45, Alessio Soldano wrote:
ResponseTime(*)Interceptor
--------------------------------------
* 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).
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 readability?
*
same reasoning for
AbstractResponseTimeInterceptor#getOperationCounterName, plus
please use StringBuilder instead of StringBuffer, as that's not
accessed concurrently.
I wrote this wrong. It's a local variable and doesn't have some
concurrent issue,so StringBuilder will be fine, no need StringBuff.
* 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)"
I'll make this change.
ResponseTimeCounter
-------------------------------
* 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?
Looked at this class again. The totalHandlingTime, minHandlingTime
and maxHandlingTime should be all atomic variable.
* At
line 87, did you really want to use "|" or "||" ?
typo !
* 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...)
no. The oneway message will go into several interceptors and
consume some time to finish the process.
* 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])
It should be all atomic variable here.
* 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])
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
think compareAndSet() can help solve this problem ?
EndpointAssociationInterceptor
-------------------------------------------
* We should probably set the service counter name in the exchange
only when the statistics are enabled.
I'll refactor this.
EndpointMetricsCXFAdapterImpl
--------------------------------------------
* counterRepo and counter members should be final, while there's
no need for keeping a reference to the objectName
Make it as method local final variable will be better.
Cheers,
Jim