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.
