Not sure I follow the answer. The intent of SystemInstance was to create a bucket separate from System.getProperties() — explicitly to not merge them together.
If there was some desire to modify actual System properties, it seems we can easily do this outside the SystemInstance. -David > On Nov 11, 2015, at 3:08 PM, Romain Manni-Bucau <[email protected]> wrote: > > 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 <[email protected]>: > >> 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 >> >>
