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]> 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. >>>> >>>> >>>> >> >
