Andrey, Kindly let us know what needs to be tested for your feature in the nightly.
Thanks Chaitanya M Bhatt On Mon, Dec 15, 2014 at 9:22 AM, Philippe Mouawad < [email protected]> 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. > 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 <[email protected]> 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 <[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. >
