++1 - This is really good! On Thu, Jun 2, 2016 at 12:16 PM, Udo Kohlmeyer <[email protected]> wrote:
> John: Perhaps the (interface) name can be simplified to > ConfigurationProperties too. > > Funnily enough I initially had called it SystemConfigurationProperties, > but later renamed it because it felt too generic. > > Dan: 3) DistributedSystem has a ton of javadocs describing each property > and what it does. I wonder if those javadocs should move to the constants > in this interface instead? > > +1, Currently the Javadoc links back to the DistributedSystem javadoc, > also happy to move that. > > DAN: 1) Is the idea with the change that all code should reference the > constants in DistributedSystemConfigProperties. For example, I should use > DistributedSystemConfigProperties.CACHE_XML_FILE when writing a test, > rather than DistributionConfig.CACHE_XML_FILE? In that case, maybe > DistributionConfig should not extend DistributedSystemConfigProperties? > > Correct, when referencing CACHE_XML_FILE, it should come from DSCP. The > refactor started with the implementation of the interface, but we should > cut the ties. Also with the current refactor I've tried to do minimally > invasive surgery. > Step1: was to extract all the properties into a single location and > *hopefully* fix all code referencing it. > Step2: Investigate the possibility to move/refactor the attribute > configuration logic (attr type,min,max,default). Currently none of that > information is directly linked to the DSCP properties. > > Bruce: Also, CacheFactory has two methods that should point to this new > class: CacheFactory(Properties) and set(String,String). > > Good point, we should change those links. I cannot help other than to ask > the question, should the ConfigurationProperties not be an Enum? Then the > */set(String,Sting)/* could become /*set(ConfigurationProperty,String) > */which could make it even easier to configure the system when using the > API. If we were to use the enum, we could set up (type,min,max,default) in > a single location for consumption and documentation. > > --Udo > > > On 3/06/2016 4:34 am, Darrel Schneider wrote: > >> +1 to naming it "ConfigurationProperties" >> +1 to moving the javadocs >> >> >> On Thu, Jun 2, 2016 at 10:46 AM, John Blum <[email protected]> wrote: >> >> Perhaps the (interface) name can be simplified to ConfigurationProperties >>> too. Not all properties necessarily involve specifically the distributed >>> system configuration, but rather the overall Geode system configuration >>> (Cache, Management, HTTP Service(s), Clients, etc). >>> >>> On Thu, Jun 2, 2016 at 10:27 AM, Dan Smith <[email protected]> wrote: >>> >>> First off - nice job, these constants should have been available a >>>> long time ago! >>>> >>>> One question a couple of comments: >>>> >>>> 1) Is the idea with the change that all code should reference the >>>> constants in DistributedSystemConfigProperties. For example, I should >>>> use DistributedSystemConfigProperties.CACHE_XML_FILE when writing a >>>> test, rather than DistributionConfig.CACHE_XML_FILE? In that case, >>>> maybe DistributionConfig should not extend >>>> DistributedSystemConfigProperties? >>>> >>>> 2) Add @since Geode 1.0 to this interface, as well as some javadocs >>>> describing to users what's contained in this interface. >>>> >>>> 3) DistributedSystem has a ton of javadocs describing each property >>>> and what it does. I wonder if those javadocs should move to the >>>> constants in this interface instead? >>>> >>>> -Dan >>>> >>>> >>>> On Wed, Jun 1, 2016 at 5:17 PM, Udo Kohlmeyer <[email protected]> >>>> wrote: >>>> >>>>> Hi there, >>>>> >>>>> As per GEODE-1377 <https://issues.apache.org/jira/browse/GEODE-1377> >>>>> >>>> I've >>>> >>>>> refactored DistributionConfig to extract all public system properties >>>>> >>>> into a >>>> >>>>> public DistributedSystemConfigProperties interface. >>>>> >>>>> With that refactor I've touched a significant amount of classed and I, >>>>> >>>> in >>> >>>> advance, apologize for the merging. >>>>> >>>>> I've have tried to find all references to all the properties that we >>>>> >>>> use >>> >>>> within Geode (be it main and test). >>>>> >>>>> For future development, if we are to use and system properties, that we >>>>> >>>> use >>>> >>>>> the provided definition for that property. That will help us to >>>>> >>>> understand >>>> >>>>> where the properties are used and will make it easier if we need to >>>>> >>>> refactor >>>> >>>>> anything in the future. >>>>> >>>>> I've also tried to find every reference to "gemfire." and replaced it >>>>> >>>> with >>>> >>>>> its defined counterpart. This should hopefully help us if we were to >>>>> >>>> move >>> >>>> away from property definitions like "gemfire.locators" to >>>>> >>>> "geode.locators". >>>> >>>>> If someone were to find that I had missed an obvious property to please >>>>> refactor and resolve that issue. >>>>> >>>>> In addition to this effort I have noticed that we use a large amount of >>>>> properties to configure functionality. I respectfully request from all >>>>> developers that if you were to work on some code that uses "internal" >>>>> properties to maybe pull it out into a common interface which can be >>>>> >>>> reused >>>> >>>>> throughout the code base. >>>>> >>>>> --Udo >>>>> >>>>> >>>>> >>> >>> -- >>> -John >>> 503-504-8657 >>> john.blum10101 (skype) >>> >>> > -- ~/William
