On 5 January 2015 at 15:35, Andrey Pokhilko <[email protected]> wrote: > What to do with test failure messages like this: > [java] Loading file testfiles/AssertionTestPlan.jmx and saving it > back changes its size from 6214 to 6306. > [java] Number of lines changes from 121 to 123 > > Should I modify the JMXes in tests?
Depends why the jmx has changed. It may indicate a code bug, e.g. a new property does not have the correct default so is being saved to the JMX. > > And this: > [java] 1) > checkI18n(org.apache.jmeter.resources.PackageTest)junit.framework.AssertionFailedError: > 1 missing labels, labels missing:Missing labels in > bundle:org/apache/jmeter/resources/messages_fr.properties > [java] save_connecttime=Save Connect Time > [java] table_visualizer_connect=Connect > [java] view_results_connect_time=Connect Time: > > I don't know French, sorry :( And I wonder why it requires it... If the properties are used anywhere then French translations are needed (unless they are just numbers and symbols like 99%). Milamber or Philippe can provide the translation if needed. > 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. >>>>>>>>> >>>>>>>>> >>>>>>>>> >>> >
