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 <ra0...@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 <seb...@gmail.com>: > > > On 25 March 2016 at 11:12, Antonio Gomes Rodrigues <ra0...@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 <seb...@gmail.com>: > > > > > >> On 23 March 2016 at 19:31, Philippe Mouawad < > philippe.moua...@gmail.com > > > > > >> wrote: > > >> > On Wed, Mar 23, 2016 at 7:41 PM, sebb <seb...@gmail.com> wrote: > > >> > > > >> >> On 23 March 2016 at 18:23, Philippe Mouawad < > > philippe.moua...@gmail.com > > >> > > > >> >> wrote: > > >> >> > On Wed, Mar 23, 2016 at 4:16 PM, sebb <seb...@gmail.com> wrote: > > >> >> > > > >> >> >> On 23 March 2016 at 15:02, Philippe Mouawad < > > >> philippe.moua...@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 <seb...@gmail.com> > wrote: > > >> >> >> > > > >> >> >> >> On 22 March 2016 at 19:12, Philippe Mouawad < > > >> >> philippe.moua...@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, <s...@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.
