On Sun, Dec 7, 2014 at 1:38 PM, Andrey Pokhilko <[email protected]> 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 < > [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. > >> > >> > >> > > > > -- Cordialement. Philippe Mouawad.
