You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Udo Kohlmeyer <uk...@pivotal.io> on 2016/06/02 00:17:35 UTC
DistributionConfig and Geode system properties
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
Re: DistributionConfig and Geode system properties
Posted by Anthony Baker <ab...@pivotal.io>.
Since this an addition to the public API (albeit small) please make sure to review and offer feedback.
We should consider how future changes (property additions, removals, renames, defaults) will impact backwards compatibility.
Thanks,
Anthony
> On Jun 1, 2016, at 5:17 PM, Udo Kohlmeyer <uk...@pivotal.io> 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
>
>
Re: DistributionConfig and Geode system properties
Posted by Bruce Schuchardt <bs...@pivotal.io>.
+1 for moving the javadocs
Also, CacheFactory has two methods that should point to this new class:
CacheFactory(Properties) and set(String,String).
Le 6/2/2016 � 10:27 AM, Dan Smith a �crit :
> 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
Re: DistributionConfig and Geode system properties
Posted by William Markito <wm...@pivotal.io>.
++1 - This is really good!
On Thu, Jun 2, 2016 at 12:16 PM, Udo Kohlmeyer <uk...@pivotal.io>
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 <jb...@pivotal.io> 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 <ds...@pivotal.io> 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 <uk...@pivotal.io>
>>>> 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
Re: DistributionConfig and Geode system properties
Posted by Udo Kohlmeyer <uk...@pivotal.io>.
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 <jb...@pivotal.io> 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 <ds...@pivotal.io> 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 <uk...@pivotal.io>
>>> 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)
>>
Re: DistributionConfig and Geode system properties
Posted by Darrel Schneider <ds...@pivotal.io>.
+1 to naming it "ConfigurationProperties"
+1 to moving the javadocs
On Thu, Jun 2, 2016 at 10:46 AM, John Blum <jb...@pivotal.io> 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 <ds...@pivotal.io> 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 <uk...@pivotal.io>
> > 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)
>
Re: DistributionConfig and Geode system properties
Posted by John Blum <jb...@pivotal.io>.
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 <ds...@pivotal.io> 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 <uk...@pivotal.io>
> 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)
Re: DistributionConfig and Geode system properties
Posted by Dan Smith <ds...@pivotal.io>.
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 <uk...@pivotal.io> 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
>
>