Re: [jbossws-dev] Fwd: EndpointMetrics Refactor
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
Attachments:
- attachment.html (text/html — 5.6 KB)