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 <[email protected]>: > On 25 March 2016 at 11:12, Antonio Gomes Rodrigues <[email protected]> > 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 <[email protected]>: > > > >> On 23 March 2016 at 19:31, Philippe Mouawad <[email protected] > > > >> wrote: > >> > On Wed, Mar 23, 2016 at 7:41 PM, sebb <[email protected]> wrote: > >> > > >> >> On 23 March 2016 at 18:23, Philippe Mouawad < > [email protected] > >> > > >> >> wrote: > >> >> > On Wed, Mar 23, 2016 at 4:16 PM, sebb <[email protected]> wrote: > >> >> > > >> >> >> On 23 March 2016 at 15:02, Philippe Mouawad < > >> [email protected] > >> >> > > >> >> >> 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 <[email protected]> wrote: > >> >> >> > > >> >> >> >> On 22 March 2016 at 19:12, Philippe Mouawad < > >> >> [email protected] > >> >> >> > > >> >> >> >> 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, <[email protected]> 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. > >> >
