Ok, I commited it without French. But I have no mail after the commit... Andrey Pokhilko
On 12/15/2014 08:22 PM, Philippe Mouawad 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. >>>>>>>> >>>>>>>> >>>>>>>> >>
