Hi, Sounds ok for commit for me. Regards
On Sunday, December 7, 2014, Andrey Pokhilko <[email protected]> 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 <[email protected] > <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 < > >> [email protected] <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 < > >>>> [email protected] <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 < > >>>>> [email protected] <javascript:;>> wrote: > >>>>> > >>>>>> Nice, will try to review this we. > >>>>>> > >>>>>> > >>>>>> On Friday, December 5, 2014, Rainer Jung <[email protected] > <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 <[email protected] > <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.
