Forgot to mention that I'm leaving for holidays Monday morning for one week without computer nor internet access… :)
Thanks -Vincent On Oct 27, 2012, at 7:13 PM, Vincent Massol <[email protected]> wrote: > Hi Andreas, > > On Oct 26, 2012, at 2:07 PM, Andreas Jonsson <[email protected]> wrote: > >> Hi Everyone, >> >> I would like you to vote on merging the feature-execution-context >> branches of commons and platform before the release of 4.3M2: >> >> https://github.com/xwiki/xwiki-commons/compare/master...feature-execution-context-metadata >> https://github.com/xwiki/xwiki-platform/compare/master...feature-execution-context-metadata >> >> Explanation of the feature: >> >> The execution context is a simple map that binds a string to an object. >> This can be compared to how variables are handled by scripting languages >> such as bash, where an assignment brings the variable into existance: >> >> $ my_variable="some value" >> >> In the case of the execution context, the syntax is: >> >> context.setProperty("my_variable", "some value"); >> >> This feature is about adding property declarations, where a property can >> be associated with attributes that controls how the execution context >> and execution context manager handles the property. The general idea >> can, once again, be compared to bash and how it is possible to declare >> variables there: >> >> $ declare -ru my_variable="read only, forced upper case" >> >> Of course, the set of attributes that are interesting to us is different >> from bash. Currently the feature branch have support for declaring >> properties with these attributes: >> >> * Final - The value may not be updated within the the execution >> context. Default: false. >> >> * Inherited - The property will be inherited from the current >> context when replacing the execution context within the current scope. >> Default: false. > >> * Cloned value - Also clone the value when the execution context is >> cloned. Default: false. > > ATM I believe that all our properties are "cloned" since when we clone the > context they are there. Won't this cause a backward compat issue? Shouldn't > it be true by default? > >> >> * Type - The class of the value, for typechecking when setting >> the value. Default: null (unchecked). >> >> * Non-null - The value may not be null, checked when setting the >> value. Default: false. >> >> Example declaration: >> >> ExecutionContextProperty property = new >> ExecutionContextProperty("my_variable"); >> property.setValue("some value"); >> property.setType(String.class); >> property.setInherited(true); >> context.declareProperty(property); > > The API is very verbose just to declare one property… IMO it would be nice to > have something more compact. > > As I've already commented I'd personally have preferred that the > ExecutionContextProperty and properties in general be immutable and set with > one call to context.setProperty(key, ECP) (i.e. no need to declare IMO). I > find it more straightforward and prevents forgetting to call declareProperty, > which becomes really critical to get the right behavior you want. > >> The property value may be updated and the property may be removed and >> redeclared, unless declared 'final': >> >> context.setProperty("my_variable", "some new value"); >> context.removeProperty("my_variable"); > > In general immutability is better in term of performance/implementation. > >> Additional attributes may be added later. This feature is also >> backwards compliant, in that implicit declaration by just setting a >> value is allowed. (Although we might want to make explicit declarations >> mandatory in the future.) >> >> Within the scope of a request, or the life time of a thread executing >> some administrative task (e.g., the lucene index updater) the basic life >> cycle of the execution context is the following: >> >> @Inject >> Execution execution; > > No need for this injection in your code below :) > >> @Inject >> ExecutionContextManager ecm; >> >> ExecutionContext context = new ExecutionContext(); >> ecm.initialize(context); >> try { >> // Do work >> } finally { >> ecm.removeContext(); >> } >> >> Within the life cycle of the "root" execution context, we may push a new >> execution context, which may either be a clean context, or a clone of >> the current context: >> >> // Pushing a clone >> >> ExecutionContext context = ecm.clone(execution.getContext()); > > <brainstorming mode> > Actually I wonder why we have both Execution and ECM. Shouldn't we have just > one? > > Also I wonder why we have to call ecm.clone() instead of implementing clone() > in ExecutionContext so that we would have: > > ExecutionContext context = execution.getContext(); > execution.pushContext(context.clone()) > > And to create an EC: > ExecutionContext ec = execution.createContext(); > > Ok maybe we would have a hard time with backward compat with this but for the > sake of the discussion, would that be something better? > </brainstorming mode> > >> execution.pushContext(context); >> try { >> // Do work >> } finally { >> execution.popContext(); >> } >> >> // Pushing a clean context >> >> ExecutionContext context = new ExecutionContext(); >> execution.pushContext(context); >> try { >> // Do work >> } finally { >> execution.popContext(); >> } >> >> Component authors that needs to place a value in the execution context >> provides an initializer that declares the variable and sets an initial >> value. >> >> The attributes 'final', 'cloned value', and 'inherited' lets component >> authors control how the value is managed during the lifecycle of the >> root execution context. >> >> The attributes 'type' and 'non-null' provides some runtime assertions to >> catch some errors earlier. >> >> So to summarize, this feature: >> >> 1. is a convenient mechanism for managing properties in the execution >> context, >> >> 2. provides some validation of the property values, >> >> 3. improves performance slightly by avoiding unnecessary cloning. >> >> For more information, see the proposal thread: >> >> http://xwiki.475771.n2.nabble.com/PROPOSAL-Execution-context-property-declarations-and-property-metadata-attributes-td7581766.html > > +0 from me after we agree on the items above. > > I'd really like to get feedback from other committers before you push this > since it's a really critical change (the Execution/EC is key and is going to > become even more and more important as we move away from XWikiContext). > > Thanks > -Vincent > _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
