jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Philippe Mouawad <philippe.moua...@gmail.com>
Subject Re: svn commit: r1532085 - /jmeter/trunk/src/core/org/apache/jmeter/control/TransactionController.java
Date Sat, 23 Nov 2013 23:43:45 GMT
On Sun, Nov 24, 2013 at 12:10 AM, sebb <sebbaz@gmail.com> wrote:

> On 23 November 2013 22:59, Philippe Mouawad <philippe.mouawad@gmail.com>
> wrote:
> > Hello sebb,
> > My answers below.
> >
> >
> > On Sat, Nov 23, 2013 at 6:17 PM, sebb <sebbaz@gmail.com> wrote:
> >
> >> On 23 November 2013 16:22, Philippe Mouawad <philippe.mouawad@gmail.com
> >
> >> wrote:
> >> > On Sat, Nov 23, 2013 at 4:41 PM, sebb <sebbaz@gmail.com> wrote:
> >> >
> >> >> On 23 November 2013 14:03, Philippe Mouawad <
> philippe.mouawad@gmail.com
> >> >
> >> >> wrote:
> >> >> > Hello,
> >> >> > What about introducing a property called:
> >> >> >
> >> >> >    - transaction_controller.include_processing_and_timers
> >> >> >
> >> >> >
> >> >> > It would allow users who have existing plans to stay with existing
> >> >> > behaviour by setting:
> >> >> >
> >> >> >    - transaction_controller.include_processing_and_timers=true
> >> >> >
> >> >> >
> >> >> > And default value would be false.
> >> >>
> >> >> That would change the behaviour for existing tests.
> >> >>
> >> > No if user sets
> >>  transaction_controller.include_processing_and_timers=true,
> >> > everything remains as currently.
> >> > We add a Incompatible change in changes.xml and everything is fine.
> >>
> >> Yes, I understand that, but it still requires existing users to update
> >> their installation.
> >> Whereas if the default is true, only users who want the new behaviour
> >> need to change their settings.
> >>
> >
> > See https://issues.apache.org/bugzilla/show_bug.cgi?id=55816 attached
> Test
> > Plan.
>
> Not sure that's relevant here. Seems like an obvious bug to me if the
> TC does not honour the setting of the include check-box.
>
> It was just to point to reported response times when Include is checked.
In Test plan I simulate processing that takes more than 1 second, and you
can see impact on reported response time.


> > If the default is true, do you find it OK that users need to update their
> > installation to have the best behaviour from JMeter when using TC ?
>
> I don't agree that it is necessarily best behaviour to ignore timers.
>

I usually use (And I don't think I am alone) TC as a wrapper to samplers
(not always, but whenever I have more than one sample in a business
Transaction).
If I don't uncheck Include... then it would mean I would be reporting
response time degraded by Post Processors behaviour (which are always
present for Response Assertions for example).

>
> > I don't. Default should be the best ones, and backward compatibility
> should
> > not be more important than having the right behaviour.
>
> Again, I don't agree that the proposed behaviour is more correct than
> the current behaviour.
>
> > I find it already very good that we introduce a special property to keep
> > old behaviour.
>
> Fine, but unless the old behaviour is clearly wrong - which I dispute
> - the default should be to keep the old behaviour.
>
> >
> >>
> >> >
> >> >> I'm not keen on changing the behaviour unless it is clearly
> incorrect.
> >> >> I don't think that is the case here.
> >> >>
> >> > It is clearly incorrect because it reports in overall response time of
> >> > Transaction Controller the time taken by post processors (I agree if
> it
> >> > reported only timers it would be acceptable) .
> >> >
> >>
> >> It's still the overall time for the TC children.
> >>
> >
> > Yes but do you agree that a user only wants cumulated response time
> > excluding any JMeter processing.
> >
> >
> >> If PostProcessors are taking a significant time to run, then that is
> >> something to be looked at separately.
> >> And there may be no PostProcessors.
> >>
> >>
> >
> >
> >> >
> >> >
> >> >>
> >> >> > By the way this impact should leed us to change something we've
> been
> >> >> doing
> >> >> > up till now which is not to write the value of a property if it
is
> >> equal
> >> >> to
> >> >> > default.
> >> >> > If we had not done this up until now , we could have changed this
> >> default
> >> >> > with no impact .
> >> >>
> >> >> I don't think that is true.
> >> >>
> >> >> AFAICT it's possible for the JMX to have one default and the property
> >> >> another.
> >> >> However it would probably require the code to check whether the test
> >> >> element is brand new or has been created from the JMX.
> >> >>
> >> >
> >> > My proposition is to always write the value wether equals or not to
> >> > default.
> >>
> >> Which makes the JMX files much larger.
> >> Also when the JMX file is saved it will change; if users store their
> >> JMX files in an SCM system, it will generate unnecessary changes.
> >>
> >> >>
> >> >> It would also have to be carefully documented as we have not done
> this
> >> so
> >> >> far.
> >> >>
> >> >> > Regards
> >> >> > Philippe
> >> >> >
> >> >> >
> >> >> >
> >> >> > On Tue, Oct 15, 2013 at 4:46 PM, sebb <sebbaz@gmail.com>
wrote:
> >> >> >
> >> >> >> On 14 October 2013 22:05, Philippe Mouawad <
> >> philippe.mouawad@gmail.com>
> >> >> >> wrote:
> >> >> >> > The problem is that if you change it , then automatically
it
> will
> >> >> change
> >> >> >> > behaviour of current scripts I think.
> >> >> >>
> >> >> >> Yes.
> >> >> >>
> >> >> >> > As when reloading existing script,
> >> >> >> > getPropertyAsBoolean(INCLUDE_TIMERS,
> >> >> DEFAULT_VALUE_FOR_INCLUDE_TIMERS);
> >> >> >> > will return DEFAULT_VALUE_FOR_INCLUDE_TIMERS , which
will be
> false
> >> >> while
> >> >> >> in
> >> >> >> > fact while its previous meaning when user saved his script
was
> >> true.
> >> >> >>
> >> >> >> The value of the default cannot be changed without affecting
> existing
> >> >> >> test plans.
> >> >> >>
> >> >> >> > See https://issues.apache.org/bugzilla/show_bug.cgi?id=55498
> >> >> >> >
> >> >> >> > In my opinion this should be changed adding an Incompatible
> change
> >> >> but we
> >> >> >> > would need to accept that users who really need it have
to
> modify
> >> >> their
> >> >> >> > script.
> >> >> >> >
> >> >> >> > +1 for this breaking change.
> >> >> >> >
> >> >> >> > Regards
> >> >> >> > Philippe
> >> >> >> >
> >> >> >> >
> >> >> >> >
> >> >> >> >
> >> >> >> > On Mon, Oct 14, 2013 at 10:56 PM, Milamber <milamber@apache.org
> >
> >> >> wrote:
> >> >> >> >
> >> >> >> >>
> >> >> >> >> Le 14/10/2013 21:47, pmouawad@apache.org a ecrit
:
> >> >> >> >>
> >> >> >> >>  Author: pmouawad
> >> >> >> >>> Date: Mon Oct 14 20:47:58 2013
> >> >> >> >>> New Revision: 1532085
> >> >> >> >>>
> >> >> >> >>> URL: http://svn.apache.org/r1532085
> >> >> >> >>> Log:
> >> >> >> >>> Add constant
> >> >> >> >>>
> >> >> >> >>
> >> >> >> >> Perhaps, It would be great to transform this default
value to a
> >> >> property
> >> >> >> >> for allow user to change this.
> >> >> >> >>
> >> >> >> >>
> >> >> >> >>
> >> >> >> >>
> >> >> >> >>
> >> >> >> >>> Modified:
> >> >> >> >>>      jmeter/trunk/src/core/org/**apache/jmeter/control/**
> >> >> >> >>> TransactionController.java
> >> >> >> >>>
> >> >> >> >>> Modified: jmeter/trunk/src/core/org/**apache/jmeter/control/**
> >> >> >> >>> TransactionController.java
> >> >> >> >>> URL:
> http://svn.apache.org/viewvc/**jmeter/trunk/src/core/org/**
> >> >> >> >>> apache/jmeter/control/**TransactionController.java?**
> >> >> >> >>> rev=1532085&r1=1532084&r2=**1532085&view=diff<
> >> >> >>
> >> >>
> >>
> http://svn.apache.org/viewvc/jmeter/trunk/src/core/org/apache/jmeter/control/TransactionController.java?rev=1532085&r1=1532084&r2=1532085&view=diff
> >> >> >> >
> >> >> >> >>>
> ==============================**==============================**
> >> >> >> >>> ==================
> >> >> >> >>> ---
> >> >> >>
> >> >>
> >>
> jmeter/trunk/src/core/org/**apache/jmeter/control/**TransactionController.java
> >> >> >> >>> (original)
> >> >> >> >>> +++
> >> >> >>
> >> >>
> >>
> jmeter/trunk/src/core/org/**apache/jmeter/control/**TransactionController.java
> >> >> >> >>> Mon Oct 14 20:47:58 2013
> >> >> >> >>> @@ -53,6 +53,8 @@ public class TransactionController
exten
> >> >> >> >>>             private static final Logger log =
> LoggingManager.**
> >> >> >> >>> getLoggerForClass();
> >> >> >> >>>   +    private static final boolean
> >> >> DEFAULT_VALUE_FOR_INCLUDE_**TIMERS
> >> >> >> =
> >> >> >> >>> true; // default true for compatibility
> >> >> >> >>> +
> >> >> >> >>>       /**
> >> >> >> >>>        * Only used in parent Mode
> >> >> >> >>>        */
> >> >> >> >>> @@ -293,7 +295,7 @@ public class TransactionController
exten
> >> >> >> >>>        * @param includeTimers
> >> >> >> >>>        */
> >> >> >> >>>       public void setIncludeTimers(boolean includeTimers)
{
> >> >> >> >>> -        setProperty(INCLUDE_TIMERS, includeTimers,
true); //
> >> >> default
> >> >> >> >>> true for compatibility
> >> >> >> >>> +        setProperty(INCLUDE_TIMERS, includeTimers,
> >> >> >> >>> DEFAULT_VALUE_FOR_INCLUDE_**TIMERS);
> >> >> >> >>>       }
> >> >> >> >>>         /**
> >> >> >> >>> @@ -302,6 +304,6 @@ public class TransactionController
exten
> >> >> >> >>>        * @return boolean (defaults to true for
backwards
> >> >> compatibility)
> >> >> >> >>>        */
> >> >> >> >>>       public boolean isIncludeTimers() {
> >> >> >> >>> -        return getPropertyAsBoolean(INCLUDE_**TIMERS,
true);
> >> >> >> >>> +        return getPropertyAsBoolean(INCLUDE_**TIMERS,
> >> >> >> >>> DEFAULT_VALUE_FOR_INCLUDE_**TIMERS);
> >> >> >> >>>       }
> >> >> >> >>>   }
> >> >> >> >>>
> >> >> >> >>>
> >> >> >> >>>
> >> >> >> >>>
> >> >> >> >>
> >> >> >> >
> >> >> >> >
> >> >> >> > --
> >> >> >> > Cordialement.
> >> >> >> > Philippe Mouawad.
> >> >> >>
> >> >> >
> >> >> >
> >> >> >
> >> >> > --
> >> >> > Cordialement.
> >> >> > Philippe Mouawad.
> >> >>
> >> >
> >> >
> >> >
> >> > --
> >> > Cordialement.
> >> > Philippe Mouawad.
> >>
> >
> >
> >
> > --
> > Cordialement.
> > Philippe Mouawad.
>



-- 
Cordialement.
Philippe Mouawad.

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message