jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: svn commit: r1736119 - /jmeter/trunk/src/components/org/apache/jmeter/visualizers/backend/BackendListener.java
Date Wed, 23 Mar 2016 22:23:05 GMT
On 23 March 2016 at 19:31, Philippe Mouawad <philippe.mouawad@gmail.com> wrote:
> On Wed, Mar 23, 2016 at 7:41 PM, sebb <sebbaz@gmail.com> wrote:
>
>> On 23 March 2016 at 18:23, Philippe Mouawad <philippe.mouawad@gmail.com>
>> wrote:
>> > On Wed, Mar 23, 2016 at 4:16 PM, sebb <sebbaz@gmail.com> wrote:
>> >
>> >> On 23 March 2016 at 15:02, Philippe Mouawad <philippe.mouawad@gmail.com
>> >
>> >> wrote:
>> >> > I guess the clean way would have been to do something like this (not
>> >> > tested) to avoid saving the arguments that are equal to default
>> values:
>> >>
>> >> It's only the NEW argument that needs to be omitted if it is the
>> default.
>> >>
>> >
>> > That's why it's a bit hacky.
>>
>> It's the standard way we deal with new properties.
>> The defaults are not saved to the JMX file.
>>
>
> Thank you , I am aware of that ...
> What's hacky here is "why only 1 property of all other arguments is not
> saved".
>
>>
>> The only thing slightly hacky is where it is currently implemented.
>> A better solution is awaited.
>
> Please help to find that rather than arguing it cannot be done.
>>
>
> Isn't it what I am doing just 1 sentence after this one ? Even if am joking
> (you don't taste it I know) about my own proposal.
> Try to joke sometimes, it will be funnier than being bad tempered ...
>
>
>> > We could introduce a new method getArgumentsNotToSaveIfEqualToDefault()
>> :-)
>> > in interface but I find it strange
>>
>> There has to be another way that is cleaner.
>>
>
> I'll let you find it...
>
>>
>> >>
>> >> Note that the code already adds the new argument if it is not present
>> >> in the JMX.
>> >> The code just needs to omit it when writing the JMX.
>> >> This is symmetrical as far as that argument is concerned.
>> >> The fact that the other arguments are always saved in the JMX is
>> >> arguably a bug in the orginal release, but we definitely cannot change
>> >> that now.
>> >>
>> >> I found a way of doing it that is not ideal, but at least it works.
>> >>
>> >> >     public void setArguments(Arguments args) {
>> >> >         try {
>> >> >             BackendListenerClient client = (BackendListenerClient)
>> >> > Class.forName(getClassname(), true,
>> >> >
>> >> > Thread.currentThread().getContextClassLoader()).newInstance();
>> >> >             Arguments defaultArgs = client.getDefaultParameters();
>> >> >             PropertyIterator iter = defaultArgs.iterator();
>> >> >             while (iter.hasNext()) {
>> >> >                 Argument defaultArg = (Argument)
>> >> > iter.next().getObjectValue();
>> >> >                 args.removeArgument(defaultArg.getName(),
>> >> > defaultArg.getValue());
>> >> >             }
>> >> >         } catch (InstantiationException | IllegalAccessException|
>> >> > ClassNotFoundException e) {
>> >> >         }
>> >> >
>> >> >         setProperty(new TestElementProperty(ARGUMENTS, args));
>> >> >     }
>> >> >
>> >> >
>> >> > But doing this now is too late
>> >>
>> >> It's not too late.
>> >>
>> >
>> > Why not make it clean and accept the minor issue to tell user his plan
>> has
>> > changed.
>>
>> Because this will keep happening unless we find a solution.
>>
> It will for people loading a < 3.0.
> Once we switch to 3.1 and superior, it will be fixed
>
>>
>> > This way it will be fixed for next version if we enhance component of it
>> > new client implementation are introduced.
>>
>> No idea what that sentence means.
>>
>
> What I call clean is not to save the properties/arguments that are equal to
> defaults.
> So it will introduce this annoyance for script loaded from < 3.0, but one
> fixed it will be for scripts loaded from 3.0 and above.

I see now.

So you want to break the JMX compatibility once, and then not do so
again, even if more properties are added?

That is another approach, but I prefer not to break compatibility at all.
I regard compatibility as much more important than clean code
(whatever that may mean).

But at least there seems to be agreement not to cause unnecessary
changes to the JMX file between versions.

>
>> > Note we have same issue with AbstractJavaSamplerClient but it's less
>> > impacting as we don't provide any that's feature rich.
>> >
>> >
>> >>
>> >> > or we could do it and accept that plan changes on load.
>> >>
>> >> > But doing it only for the new field looks strange if not ugly to me
>> and
>> >>
>> >> > anyway I don't think it should be done here as client can be of any
>> class
>> >> > not just GraphiteBackendListenerClient
>> >>
>> >> Agreed it would be better to do it only for that client.
>> >>
>> >> Suggestions welcome.
>> >>
>> >> > Regards
>> >> > Philippe
>> >> >
>> >> > On Tue, Mar 22, 2016 at 9:09 PM, sebb <sebbaz@gmail.com> wrote:
>> >> >
>> >> >> On 22 March 2016 at 19:12, Philippe Mouawad <
>> philippe.mouawad@gmail.com
>> >> >
>> >> >> wrote:
>> >> >> > Hello sebb,
>> >> >> > Although this fixes the issue, it seems to me as a violation
of the
>> >> >> > architecture .
>> >> >> > BackendListener should not be aware of a particular implementation
>> of
>> >> >> > BackendListenerClient : GraphiteBackendListenerClient
>> >> >>
>> >> >> If you can move the fix into GraphiteBackendListenerClient please
do
>> so.
>> >> >>
>> >> >> > Regards
>> >> >> >
>> >> >> > On Tue, Mar 22, 2016 at 1:54 AM, <sebb@apache.org> wrote:
>> >> >> >
>> >> >> >> Author: sebb
>> >> >> >> Date: Tue Mar 22 00:54:30 2016
>> >> >> >> New Revision: 1736119
>> >> >> >>
>> >> >> >> URL: http://svn.apache.org/viewvc?rev=1736119&view=rev
>> >> >> >> Log:
>> >> >> >> New fields/changed defaults cause earlier test plans to
be marked
>> as
>> >> >> >> changed
>> >> >> >> Fix BackendListener
>> >> >> >> Bugzilla Id: 59173
>> >> >> >>
>> >> >> >> Modified:
>> >> >> >>
>> >> >> >>
>> >> >>
>> >>
>> jmeter/trunk/src/components/org/apache/jmeter/visualizers/backend/BackendListener.java
>> >> >> >>
>> >> >> >> Modified:
>> >> >> >>
>> >> >>
>> >>
>> jmeter/trunk/src/components/org/apache/jmeter/visualizers/backend/BackendListener.java
>> >> >> >> URL:
>> >> >> >>
>> >> >>
>> >>
>> http://svn.apache.org/viewvc/jmeter/trunk/src/components/org/apache/jmeter/visualizers/backend/BackendListener.java?rev=1736119&r1=1736118&r2=1736119&view=diff
>> >> >> >>
>> >> >> >>
>> >> >>
>> >>
>> ==============================================================================
>> >> >> >> ---
>> >> >> >>
>> >> >>
>> >>
>> jmeter/trunk/src/components/org/apache/jmeter/visualizers/backend/BackendListener.java
>> >> >> >> (original)
>> >> >> >> +++
>> >> >> >>
>> >> >>
>> >>
>> jmeter/trunk/src/components/org/apache/jmeter/visualizers/backend/BackendListener.java
>> >> >> >> Tue Mar 22 00:54:30 2016
>> >> >> >> @@ -39,6 +39,7 @@ import org.apache.jmeter.testelement.Abs
>> >> >> >>  import org.apache.jmeter.testelement.TestElement;
>> >> >> >>  import org.apache.jmeter.testelement.TestStateListener;
>> >> >> >>  import
>> org.apache.jmeter.testelement.property.TestElementProperty;
>> >> >> >> +import
>> >> >> >>
>> >> >>
>> >>
>> org.apache.jmeter.visualizers.backend.graphite.GraphiteBackendListenerClient;
>> >> >> >>  import org.apache.jorphan.logging.LoggingManager;
>> >> >> >>  import org.apache.log.Logger;
>> >> >> >>
>> >> >> >> @@ -434,6 +435,9 @@ public class BackendListener extends
Abs
>> >> >> >>       *            the new arguments. These replace any
existing
>> >> >> arguments.
>> >> >> >>       */
>> >> >> >>      public void setArguments(Arguments args) {
>> >> >> >> +        // Bug 59173 - don't save new default argument
>> >> >> >> +
>> >> >> >>
>> >> >>
>> >>
>> args.removeArgument(GraphiteBackendListenerClient.USE_REGEXP_FOR_SAMPLERS_LIST,
>> >> >> >> +
>> >> >> >>
>> GraphiteBackendListenerClient.USE_REGEXP_FOR_SAMPLERS_LIST_DEFAULT);
>> >> >> >>          setProperty(new TestElementProperty(ARGUMENTS,
args));
>> >> >> >>      }
>> >> >> >>
>> >> >> >>
>> >> >> >>
>> >> >> >>
>> >> >> >
>> >> >> >
>> >> >> > --
>> >> >> > Cordialement.
>> >> >> > Philippe Mouawad.
>> >> >>
>> >> >
>> >> >
>> >> >
>> >> > --
>> >> > Cordialement.
>> >> > Philippe Mouawad.
>> >>
>> >
>> >
>> >
>> > --
>> > Cordialement.
>> > Philippe Mouawad.
>>
>
>
>
> --
> Cordialement.
> Philippe Mouawad.

Mime
View raw message