Not sure why this change has been along this particular commit but the need was to be able to leverage the aggregation of properties tomee does on properties designed as "system properties" in system properties.
I have no issue saving initial system properties and resetting them with the SystemInstance but I would be concerned not merging system properties with system properties (intentionally phrased this way). This is widely used AFAIK cause most of libraries rely on System.getProperty and conf/system.properties - to not speak about other locations - is quite heavily used by users in such a manner in tests but in prod as well. As a summary: -1 to not merge anymore, +1 to save initial properties and restore them after (with the traditional flag to not do it if a user was relying on it in a test suite - wouldnt happen in prod I think). Romain 2015-11-11 14:59 GMT-08:00 David Blevins <david.blev...@gmail.com>: > Looks like we made as part of a routine Tomcat upgrade to change > SystemInstance so that it modifies the System.getProperties > > - commit: > https://github.com/apache/tomee/commit/d9b5ea3f7218dc4bad1ebac7b8b674823fa3fa4c#diff-f51fa1817d7f3bd166acc0ea1b4f6b69 > < > https://github.com/apache/tomee/commit/d9b5ea3f7218dc4bad1ebac7b8b674823fa3fa4c#diff-f51fa1817d7f3bd166acc0ea1b4f6b69 > > > - issue: https://issues.apache.org/jira/browse/TOMEE-740 < > https://issues.apache.org/jira/browse/TOMEE-740> > > The goal for SystemInstance was specifically to remove dependency on > System.getProperties so that embedded containers could be booted, used and > discarded without polluting the System properties. This was all done as > part of OpenEJB 1’s integration with Tomcat 4 (who feels old now?) which > allowed you to have one embedded EJB container per webapp and also > leveraged to allow cleaner Java SE integration so embedded EJB containers > could be used and discarded with no scrubbing of the system properties. > > Saw some offline discussion saying “we need to clean up the system > properties because the SystemInstance modifies them”, so thought it’s a > good time to have a chat :) > > Any feedback on: > > - What motivated the change? > - How do we rework that code? > > Ideally, we’d restore the ability to sequentially boot embedded > containers, passing in properties and not have the actual properties used > reflect some aggregate of all the containers that came before. > > > -- > David Blevins > http://twitter.com/dblevins > http://www.tomitribe.com > >
