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)
