> On March 27, 2018, 2:49 p.m., András Piros wrote: > > Interesting idea! > > > > However, in the existing approach within `ConfigurationService#loadConf()` > > following handling of system properties is implemented: > > > > * `ConfigurationService#IGNORE_SYS_PROPS` defines the system properties > > that should never be overwritten, neither from config file nor from `-D` > > option > > * `ConfigurationService#IGNORE_SYS_PROPS` contains both hard coded and > > additionally configured entries > > * `ConfigurationService#CONF_SYS_PROPS` defines a fixed set of system > > properties. When the actual system property is present, and isn't contained > > in `ConfigurationService#IGNORE_SYS_PROPS`, a `Configuration` entry is > > inserted > > * in addition to `ConfigurationService#CONF_SYS_PROPS`, all the already > > existing `Configuration` entries are checked for existence as a system > > property. If present and not contained in > > `ConfigurationService#IGNORE_SYS_PROPS`, the appropriare `Configuration` > > entry is updated > > > > I'd suggest that the handling of the extra, admin-allowed system properties > > happens as the last step, and an extra filtering should also be taken for > > absence inside `ConfigurationService#IGNORE_SYS_PROPS`. The prefix should > > also be `oozie.` > > Denes Bodo wrote: > Thank you for your explanation. I see that there is another way to solve > my problem; simply define property in site.xml with the same name as wanted > system property. > I am glad that this list fits into Oozie's spirit. However, do you mean > that "The prefix should also be oozie" should be understood as the property > names in the list should be started with oozie? > > Sorry for the late reaction, I didn't get notification about your review. > :\
You're welcome :) "The prefix should also be oozie" - I mean we shouldn't allow any system properties, but only the ones beginning with `oozie.` prefix, and not being part of `ConfigurationService#IGNORE_SYS_PROPS`, should be allowed. So yes, the property names in the system properties list should be filtered for that prefix, as well. - András ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66305/#review200046 ----------------------------------------------------------- On March 27, 2018, 11:59 a.m., Denes Bodo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66305/ > ----------------------------------------------------------- > > (Updated March 27, 2018, 11:59 a.m.) > > > Review request for oozie and András Piros. > > > Bugs: OOZIE-3199 > https://issues.apache.org/jira/browse/OOZIE-3199 > > > Repository: oozie-git > > > Description > ------- > > Let system property restriction configurable > > > Diffs > ----- > > core/src/main/java/org/apache/oozie/service/ConfigurationService.java > 618d5e6 > core/src/main/resources/oozie-default.xml 6f89258 > > > Diff: https://reviews.apache.org/r/66305/diff/1/ > > > Testing > ------- > > Tested on private Hadoop cluster. > > > Thanks, > > Denes Bodo > >
