Hi Philippe,
This is a great help to have your code review, thanks! I tried to fix
following items, please verify:
* MeasuringConnectionManager#dnsResolver removed
* Added Connect time into View Results in Table
* ATT_CONNECT_TIME changed to ct
* connectedTime and startedTime are removed, you are right about their
origin
* added License header and javadocs to MeasuringConnectionManager
* fixed NPE source by removing the connManager field
The items that I did not fix, but can fix if you will insist on it:
* I don't agree that CSV_CONNECT_TIME should be changed to
"Connection", for me "connect" is short of "connect time", and
"connection" is about "connection state" or "connection object"
* I don't think the 'PoolingClientConnectionManagerAdapter' makes our
code easier to read. The approach to make Java class names more and
more long has its limits. Finally, ask yourself, what is more
obvious in this situation - MeasuringConnectionManager or
PoolingClientConnectionManagerAdapter ?
* I did not understand what is wrong with MeasuringConnectionRequest
and MeasuringConnectionRequest classes being inner. What do you
offer instead?
* The SampleResult instance variable is the only way to have the same
style as for latency, when you just call "connectEnd". Otherwise we
need to get connectedTime and startedTime back and use simple
getter. The only change that might make sense is to use
WeakReference to shorten the lifetime of SampleResult. As I see in
the code there is no ways currently to share the same HTTPClient
between threads.
Let's discuss, maybe with more opinions from other contributors, what's
should be finally done with these items.
NPE will be discussed in a different branch of emails.
Andrey Pokhilko
On 12/06/2014 04:34 PM, Philippe Mouawad wrote:
> Hi,
> I checked , it seems OK regarding multi-threading.
>
> Few more notes:
> MeasuringConnectionManager#dnsResolver is not used
>
> Shouldn't connectTime be added to View Results in Table ?
>
> Regards
>
>
> On Sat, Dec 6, 2014 at 2:51 PM, Philippe Mouawad <[email protected]
>> wrote:
>> Hi,
>> Note there is currently a bugzilla and patch (but partly outdated due to
>> HttpSampler having been drastically reworked since that time) for this:
>>
>> - https://issues.apache.org/bugzilla/show_bug.cgi?id=48799
>>
>>
>> Regards
>>
>> On Sat, Dec 6, 2014 at 2:48 PM, Philippe Mouawad <
>> [email protected]> wrote:
>>
>>> Hi,
>>> My notes:
>>> Naming:
>>> - CSV_CONNECT_TIME , Connect should maybe be named Connection
>>> - ATT_CONNECT_TIME should maybe be ct (for ConnectTime) instead of cn
>>> - MeasuringConnectionManager should be
>>> PoolingClientConnectionManagerAdapter ?
>>> - MeasuringConnectionRequest should be ClientConnectionRequestAdapter ?
>>> - MeasuredConnection should be
>>> MeasuringConnectionManager is missing Apache License header and javadocs
>>> :-)
>>>
>>> public class MeasuringConnectionRequest should not be public static class
>>> MeasuringConnectionRequest ?
>>> Same for MeasuredConnection ?
>>>
>>> MeasuredConnection :
>>> connectedTime and startedTime are not used , I suppose it was a work in
>>> progress code that was not deleted afterwards.
>>>
>>>
>>> More in depth remarks:
>>> - Are you sure about having SampleResult instance variable of
>>> MeasuringConnectionManager ?
>>> This should be checked as I am afraid you may end up sharing SampleResult
>>> between different samples and threads.
>>> I need more time to check this.
>>>
>>> Regards
>>> Philippe M.
>>> @philmdot
>>>
>>>
>>>
>>>
>>> On Fri, Dec 5, 2014 at 9:55 PM, Philippe Mouawad <
>>> [email protected]> wrote:
>>>
>>>> Nice, will try to review this we.
>>>>
>>>>
>>>> On Friday, December 5, 2014, Rainer Jung <[email protected]>
>>>> wrote:
>>>>
>>>>> Am 03.12.2014 um 15:15 schrieb Andrey Pokhilko:
>>>>>
>>>>>> Happened to be not so much work:
>>>>>> https://github.com/apache/jmeter/pull/11/files
>>>>>>
>>>>>> Please, review it and point me at any changes needed.
>>>>>>
>>>>> I didn't review the patch but I patched a 2.12 I'm currently using to
>>>>> probe a service and it seems to run well here.
>>>>>
>>>>> Regards,
>>>>>
>>>>> Rainer
>>>>>
>>>>> On 11/29/2014 04:06 PM, sebb wrote:
>>>>>>> On 29 November 2014 at 12:14, Andrey Pokhilko <[email protected]> wrote:
>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Many times I see a sence to have connect times measured separately,
>>>>>>>> in
>>>>>>>> addition to latency that we have in SampleResult. It is important
>>>>>>>> when
>>>>>>>> measuring a time for SSL handshake and DNS resolving, when users
>>>>>>>> want to
>>>>>>>> see it separate share in total Response Time.
>>>>>>>>
>>>>>>>> Connect time is available as separate metric in Grinder and
>>>>>>>> Yandex.Tank.
>>>>>>>> The latter has following details on response time pars: connect,
>>>>>>>> send,
>>>>>>>> latency, receive. Sometimes some parts are zero, but at least there
>>>>>>>> is a
>>>>>>>> technical possibility to see when it is non-zero. It should be noted
>>>>>>>> that full breakdown would be: dns, connect, send, latency, receive.
>>>>>>>>
>>>>>>>> Send and receive times are not of great importance, IMO. And I would
>>>>>>>> cope with connect time including DNS resolve time. But having connect
>>>>>>>> time would add interesting aspect on results.
>>>>>>>>
>>>>>>> [I expect DNS resolve time might be very tricky to measure in Java]
>>>>>>>
>>>>>>> For implementation it will require adding one more property with
>>>>>>>> getters
>>>>>>>> and setters to SampleResult, modifying SampleSaveConfiguration and UI
>>>>>>>> settings to configure saving, using this new field in HTTP sampler,
>>>>>>>> TCP
>>>>>>>> sampler, maybe there are other samplers that can respect this field.
>>>>>>>>
>>>>>>> The docs would need to be updated to state whether a sampler supports
>>>>>>> the metric or not.
>>>>>>>
>>>>>>> As separate question I would raise if latency should not include
>>>>>>>> connect
>>>>>>>> time, for me it sounds logical, but changes existing behavior.
>>>>>>>>
>>>>>>> Connect time is currently included in both latency and elapsed.
>>>>>>>
>>>>>>> The simplest would be to just add connect as a separate time, but not
>>>>>>> subtract it from latency or elapsed.
>>>>>>> This would allow further analysis without changing behaviour.
>>>>>>> Maybe add an option to perform the subtraction.
>>>>>>> I don't think we should change the default behaviour.
>>>>>>>
>>>>>>> Any opinions?
>>>>>>> I can see its use and am not against it, but it needs quite a lot of
>>>>>>> work to implement fully.
>>>>>>>
>>>>>>> --
>>>>>>>> Andrey Pokhilko
>>>>>>>>
>>>> --
>>>> Cordialement.
>>>> Philippe Mouawad.
>>>>
>>>>
>>>>
>>>>
>>>
>>> --
>>> Cordialement.
>>> Philippe Mouawad.
>>>
>>>
>>>
>>
>> --
>> Cordialement.
>> Philippe Mouawad.
>>
>>
>>
>
