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: r1736119 - /jmeter/trunk/src/components/org/apache/jmeter/visualizers/backend/BackendListener.java
Date Sun, 27 Mar 2016 19:39:49 GMT
I created a bug to track this:
https://bz.apache.org/bugzilla/show_bug.cgi?id=59241

I closed 59173 <https://bz.apache.org/bugzilla/show_bug.cgi?id=59173>

Regards

On Fri, Mar 25, 2016 at 1:42 PM, Antonio Gomes Rodrigues <ra0077@gmail.com>
wrote:

> Hi Sebb,
>
> Thanks to your quick answer, now it's more cleare to me
>
> If file size is a problem, I don't think to keep xml format is a good idea
>
> Antonio
>
>
> 2016-03-25 13:26 GMT+01:00 sebb <sebbaz@gmail.com>:
>
> > On 25 March 2016 at 11:12, Antonio Gomes Rodrigues <ra0077@gmail.com>
> > wrote:
> > > Hi,
> > >
> > > Why not define a new JMX output to 3.0?
> > >
> > > If the new format save all the value (default or not), is that problem
> > > would be solved and never appear?
> >
> > No, because every time a new property is added, the JMX would change.
> > That is the main point of not saving the default value for new
> properties.
> >
> > (A secondary effect is not increasing the JMX file size).
> >
> > For people that keep test plans in a CMS, it's a nuisance if
> > unnecessary data is added between releases.
> > Makes it harder to tell what has really changed.
> >
> > > If yes, why don't have the 2 solutions for 3.0 (Save old format + Save
> > new
> > > format) and deprecated old format some release later?
> > >
> > > Antonio
> > >
> > > 2016-03-23 23:23 GMT+01:00 sebb <sebbaz@gmail.com>:
> > >
> > >> 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.
> > >>
> >
>



-- 
Cordialement.
Philippe Mouawad.

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