Didn't quite understand how the backward compatibility will be broken. If AB is operator attribute, then dt.operator.*.attr.AB is used. Now we added AB as a port attribute so it can be declared as dt.operator.*.port.*.attr.AB. How does it break backward compatibility?
I mentioned dt.attr.*.SPIN_MILLIS is valid while dt.attr.SPIN_MILLIS is not because SPIN_MILLIS is not a dag attribute. - Chandni On Tue, Sep 8, 2015 at 11:01 PM, Timothy Farkas <[email protected]> wrote: > Hi Chandni, > > Doing that would introduce backwards compatibility issues in the future. > > For example let's say we implement your change and we have an attribute AB > which exists in OperatorContext today. If a developer wanted to add AB to > PortContext in a future version, that would be a backwards incompatible > change because it would break all the apps configs which defined dt.attr.AB > (which was an acceptable way to define AB before because AB was not > ambiguous before). > > A real world example of this issue would be unifier attributes > https://malhar.atlassian.net/browse/APEX-42. Unifier attributes have all > the same attributes as Operators except their parents in a configuration > are a port ex. > dt.application.*.operator.*.outputport.*.unifier.*.attr.SPIN_MILLIS. If we > forced a different syntax for ambiguous attributes, it would be impossible > to add something like unifier attributes in a backwards compatiblechange, > because it would make previously unambiguous attribute declaration > ambiguous and would cause configuration files which worked without errors > before to start throwing errors. > > Tim > > On Tue, Sep 8, 2015 at 9:26 PM, Chandni Singh <[email protected]> > wrote: > > > Hi Tim, > > > > With respect to syntax there isn't any difference between > > dt.attr.CHECKPOINT_WINDOW_COUNT > > and dt.attr.SPIN_MILLIS. > > However behavior differs. > > Since CHECKPOINT_WINDOW_COUNT is also a DAG attr it is not treated as > > ambiguous while the later is. > > > > Seems like an application developer who sets these attributes will have > to > > refer the attributes in order to avoid any errors. > > > > Can we avoid this confusion by forcing difference in syntax? For eg. > > dt.attr.*.SPIN_MILLIS is valid while dt.attr.SPIN_MILLIS is not? > > > > Chandni > > > > On Tue, Sep 8, 2015 at 9:56 PM, Timothy Farkas <[email protected]> > > wrote: > > > > > Hi Thomas, > > > > > > If an ambiguous attribute like CHECKPOINT_WINDOW_COUNT is specified on > an > > > element which owns the attribute then that is not an ambiguous usage. > > > Essentially nothing changes for CHECKPOINT_WINDOW_COUNT. For example: > > > > > > dt.attr.CHECKPOINT_WINDOW_COUNT should only set the DAG attribute, just > > as > > > it does now. > > > dt.attr.operator.*.attr.CHECKPOINT_WINDOW_COUNT should only set the > > > operator attribute as it does now. > > > > > > Another type of ambiguous attribute is SPIN_MILLIS which is defined for > > > both ports and operators. There are three usage scenarios for this: > > > > > > 1. dt.attr.SPIN_MILLIS or dt.attr.application.*.SPIN_MILLIS > > > This will set the same SPIN_MILLIS value for all operators and ports > > > > > > 2. dt.attr.operator.*.SPIN_MILLIS > > > This will set the SPIN_MILLIS value for only operators > > > > > > 3. dt.attr.port.*.SPIN_MILLIS > > > This will set the SPIN_MILLIS value for only ports > > > > > > It could also be possible to use the fully qualified attribute name of > an > > > attribute, which is the attribute name prefixed by the class name of > the > > > Context > > > > > > 4. dt.attr.com.datatorrent.api.Context.PortContext.SPIN_MILLIS > > > This will set the attribute for ports only > > > > > > 5. dt.attr.com.datatorrent.api.Context.OperatorContext.SPIN_MILLIS > > > This will set the attribute for operators only > > > > > > 6. > > > > > > > > > dt.attr.operator.*.attr.com.datatorrent.api.Context.PortrContext.SPIN_MILLIS > > > This will set the attribute for ports only > > > > > > This changes are backwards compatible. Also two things are done to make > > it > > > easy to maintain backwards compatibility moving forward: > > > > > > - If a value is defined for an attribute ambiguously like > > > dt.attr.SPIN_MILLIS . Then the same value is set on all contexts. This > > > behavior will allow a developer to maintain backwards compatibility if > > they > > > add an existing attribute name to a new context. > > > > > > - Attributes with the same name must have the same type, if they don't > > the > > > unit tests fail. This prevents developers from accidentally making a > > > backwards incompatible change. > > > > > > Thanks, > > > Tim > > > > > > On Tue, Sep 8, 2015 at 5:40 PM, Thomas Weise <[email protected]> > > > wrote: > > > > > > > How would you handle the case where two attributes with the same name > > > exist > > > > in the hierarchy, for example CHECKPOINT_WINDOW_COUNT (DAG and > > > > OperatorContext)? > > > > > > > > Is the proposed change backward compatible? > > > > > > > > > > > > On Tue, Sep 8, 2015 at 5:44 PM, Timothy Farkas <[email protected]> > > > > wrote: > > > > > > > > > Hello all, > > > > > > > > > > This JIRA https://malhar.atlassian.net/browse/APEX-28 is > proposing a > > > > > change > > > > > to the way attributes are declared in properties files. The > proposal > > > will > > > > > allow a short form declaration of properties, and supports the > > > following > > > > > features: > > > > > > > > > > 1 - Root attribute declaration > > > > > > > > > > dt.attr.MEMORY_MB is now a valid property and will set the > MEMORY_MB > > > > > attribute for all operators > > > > > > > > > > 2 - Ambiguous property handling > > > > > > > > > > dt.attr.AUTO_RECORD will set the AUTO_RECORD attribute on both > > > operators > > > > > and ports > > > > > > > > > > 3 - Fully qualified attribute names > > > > > > > > > > Attribute names can be prefix with the name of their Context class > > > > > > > > > > dt.attr.com.datatorrent.api.Context.PortContext.AUTO_RECORD will > set > > > the > > > > > AUTO_RECORD attribute for all ports > > > > > > > > > > 4 - Child attributes can be specified on parent elements > > > > > > > > > > dt.attr.application.*.attr.MEMORY_MB will set the MEMORY_MB > attribute > > > on > > > > > all operators > > > > > > > > > > 5 - Existing methods for setting properties are supported. > > > > > > > > > > An implementation of this is pending review here: > > > > > https://github.com/apache/incubator-apex-core/pull/11 > > > > > > > > > > Please provide your feedback. > > > > > > > > > > Thanks, > > > > > Tim > > > > > > > > > > > > > > >
