jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: Introduce connect times for SampleResult?
Date Mon, 15 Dec 2014 17:33:07 GMT
On 15 December 2014 at 17:22, Philippe Mouawad
<philippe.mouawad@gmail.com> wrote:
> Hi,
> You need to update docs and component_reference.xml or any doc that
> currently mentions other reported indicators, don't forget screenshots if
> any are deprecated.
> Also fill in changes.xml with bug id at the right place, and when commiting
> put in comment the bug ID + the bugzilla full title.
>
> YOu need to commit files all in 1.

Ideally, but if you forget some files, it's OK to add these in another
commit, so long as the correct Bug id is used, and the Bugzilla issue
is updated.

The intention is to be able to easily find the changes that were made
as a result of a Bugzilla issue.

It's important that a single commit only includes directly related changes.
Unrelated changes should be done in separate commits.

If a commit contains multiple unrelated changes, it's much harder to
review, and is much harder to revert if that becomes necessary.

> Once done, take the mail your receive with commit and copy the line
> starting after author and just before commit files changes details, and put
> this in the bugzilla that you close as resolved.
>
> Regards
>
>
>
>
>
> On Mon, Dec 15, 2014 at 9:18 AM, Andrey Pokhilko <apc4@ya.ru> wrote:
>>
>> Is there anything else that I should add to commit? Changelog records
>> etc? Is there a document for commit requirements?
>>
>> Andrey Pokhilko
>>
>> On 12/15/2014 12:28 AM, Philippe Mouawad wrote:
>> > Hi,
>> > Sounds ok for commit for me.
>> >
>> > Regards
>> >
>> > On Sunday, December 7, 2014, Andrey Pokhilko <apc4@ya.ru> wrote:
>> >
>> >> Note that I made classes private static, because they seems have no use
>> >> outside connection manager. Is it OK?
>> >>
>> >> Andrey Pokhilko
>> >>
>> >> On 12/07/2014 03:37 PM, Philippe Mouawad wrote:
>> >>> On Sun, Dec 7, 2014 at 1:38 PM, Andrey Pokhilko <apc4@ya.ru
>> >> <javascript:;>> wrote:
>> >>>> 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"
>> >>>>
>> >>> Ok just a matter of opinion :-)
>> >>>
>> >>>>   * 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 ?
>> >>>>
>> >>> Ok just a matter of opinion :-)   , I named them after pattern, you
>> name
>> >>> them after their use , ok for me.
>> >>>
>> >>>
>> >>>>   * I did not understand what is wrong with MeasuringConnectionRequest
>> >>>>     and MeasuringConnectionRequest classes being inner. What do
you
>> >>>>     offer instead?
>> >>>>
>> >>> I am asking to make the public static class instead of public class.
>> >> There
>> >>> is no benefit of making them public class and you hold uselessly a
>> >>> reference to MeasuringConnectionManager instance
>> >>>
>> >>>   * 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.
>> >>>>
>> >>> Ok, as I said in the following mail there was no issue except the
>> NPE.  I
>> >>> will check new PR.
>> >>>
>> >>>> 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 <
>> >>>> philippe.mouawad@gmail.com <javascript:;>
>> >>>>>> 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 <
>> >>>>>> philippe.mouawad@gmail.com <javascript:;>> 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 <
>> >>>>>>> philippe.mouawad@gmail.com <javascript:;>>
wrote:
>> >>>>>>>
>> >>>>>>>> Nice, will try to review this we.
>> >>>>>>>>
>> >>>>>>>>
>> >>>>>>>> On Friday, December 5, 2014, Rainer Jung <rainer.jung@kippdata.de
>> >> <javascript:;>>
>> >>>>>>>> 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 <apc4@ya.ru
>> >> <javascript:;>> 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.
>> >>>>>>
>> >>>>>>
>> >>>>>>
>> >>
>>
>>
>
> --
> Cordialement.
> Philippe Mouawad.

Mime
View raw message