Hi, Obviously, I should vote +1 myself as well. So, we have 2 +1 and 1 +0. Does anyone else have an opinion on this? Maybe Thomas?
/Andreas 2012-10-29 19:31, Sergiu Dumitriu skrev: > On 10/28/2012 04:43 PM, Andreas Jonsson wrote: >> Hi Vincent, >> >> 2012-10-27 19:13, Vincent Massol skrev: >>> 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? >> No, it is not actually a compatibility issue, because the clone method >> in the execution context manager doesn't actually clone the execution >> context. This option is therefore only used when copying the properties >> when inheriting the parent context, which is a new feature. >> >> But having said that, it is possible that we should enable cloning by >> default. My thoughts on this is only that we cannot know if the the >> value can be cloned, which would be a slight complication. >> >> >>>> * 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. >> Ok. To make the API more compact, I changed it to use a builder pattern >> for declaring properties, for example: >> >> context.newProperty("key").type(String.class).nonNull().initial("value").declare(); >> >> >>> 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. >>> >> Let's see if I understand you correctly. First, I would say that >> setting a property with attributes (i.e., setProperty(key, ECP)) is >> precisely what I mean by 'declaring' a property. You are just using a >> different method name. So, I'm taking the liberty to continue to use >> the word 'declaring' to denote 'setting a property with attributes'. >> >> With this in mind, do you suggest that we forbid updating property >> values altogether (i.e., the same as declaring them final by default >> with no option to not declare them final)? Also, do you suggest >> deprecating 'ExecutionContext.setValue(String, Object)' to disallow >> implicit declarations altogether? >> >> The ability to actually replace the value object seems very useful to >> me. A simple use case is that of keeping a boolean flag in the context: >> >> context.newProperty("flag").type(Boolean.class).nonNull() >> .initial((Boolean) false).declare(); >> >> // ... >> >> if (! ((Boolean) context.getProperty("flag"))) { >> context.setProperty("flag", (Boolean) true); >> // ... >> } >> >> As of implicit declarations, I have allowed them only to maintain >> backwards compatiblity. I would be in favour of disallowing them. >> >> >>>> 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? >> Yes, the execution context manager seems redundant. >> >>> 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()) >> Given that the method ECM.clone(EC) doesn't actually clone anything, >> particularily not the execution context given as argument, I'd say that >> it should be deprecated in favor of making the execution context itself >> cloneable. >> >> >> Best Regards, >> >> /Andreas >> >> >> >>> 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). > I agree with the current state of the code, +1 for merge. > _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
