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