Start using and adding default properties #2146
Conversation
| <property name="ACM.Client" deprecated="true"/> | ||
| <property name="ACM.Server" deprecated="true"/> | ||
| <property name="ACM" class="acm"/> | ||
| <property name="ACM.Client" class="acm"/> | ||
| <property name="ACM.Server" class="acm"/> | ||
| <property name="Admin" class="objectadapter" /> | ||
| <property name="Admin.DelayCreation" /> | ||
| <property name="Admin.DelayCreation" default="0" /> |
There was a problem hiding this comment.
For Admin.Enabled, I would add a comment about the default, something like:
<!-- default is computed using ... -->
There was a problem hiding this comment.
Same for all other Admin (and more) without a specified default.
There was a problem hiding this comment.
I don't think we should start documenting this XML file too heavily. It's too hard to keep in sync with the docs.
| <property name="Admin.Logger.Properties" /> | ||
| <property name="Admin.ServerId" /> | ||
| <property name="BackgroundLocatorCacheUpdates"/> | ||
| <property name="BackgroundLocatorCacheUpdates" default="0" /> | ||
| <property name="BatchAutoFlush" deprecated="true"/> |
There was a problem hiding this comment.
default is 0 (false), right?
Should we actually eliminate deprecated properties in a follow-up PR?
There was a problem hiding this comment.
Which property are you referring to?
| <property name="Default.SourceAddress" /> | ||
| <property name="Default.Timeout" /> | ||
| <property name="Default.Timeout" default="60000" /> |
There was a problem hiding this comment.
I would add a comment: TODO: remove
config/PropertyNames.xml
Outdated
| <property name="ChangeUser" /> | ||
| <property name="ClassGraphDepthMax" /> | ||
| <property name="ClassGraphDepthMax" default="100" /> | ||
| <property name="ClientAccessPolicyProtocol" /> |
There was a problem hiding this comment.
Remove ClientAccessPolicyProtocol.
| <property name="ClientAccessPolicyProtocol" /> | ||
| <property name="Compression.Level" /> | ||
| <property name="Compression.Level" default="1" /> | ||
| <property name="Config" /> | ||
| <property name="Connection" class="connection"/> | ||
| <property name="ConsoleListener" /> |
There was a problem hiding this comment.
The default for ConsoleListener is undocumented. It's 1 in the code (C# only).
There was a problem hiding this comment.
I opened an issue to fix the docs
| <property name="Registry.RequireNodeCertCN" /> | ||
| <property name="Registry.RequireReplicaCertCN" /> | ||
| <property name="Registry.Server" class="objectadapter" /> | ||
| <property name="Registry.SessionFilters" /> | ||
| <property name="Registry.SessionFilters" default="0" /> |
There was a problem hiding this comment.
For all the Trace below, the default is not 0?
There was a problem hiding this comment.
Probably, I haven't gotten to properties that we don't read plainly.
For instance grepping for Node.Trace.Activator doesn't show anything.
❯ rg -F Node.Trace.Activator
config/PropertyNames.xml
504: <property name="Node.Trace.Activator" />
scripts/IceGridUtil.py
131: "IceGrid.Node.Trace.Activator": 0,
java/src/Ice/src/main/java/com/zeroc/IceInternal/PropertyNames.java
810: new Property("IceGrid.Node.Trace.Activator", false, "", false, null),
csharp/src/Ice/Internal/PropertyNames.cs
784: new(@"IceGrid.Node.Trace.Activator", false, "", false, null),
cpp/config/icegridnode.cfg
58:IceGrid.Node.Trace.Activator=3
cpp/src/Ice/PropertyNames.cpp
802: IceInternal::Property("IceGrid.Node.Trace.Activator", false, "", false, nullptr),
There was a problem hiding this comment.
ice/cpp/src/IceGrid/TraceLevels.cpp
Line 47 in c4a96d3
| @@ -301,64 +301,64 @@ generated from the section label. | |||
| <class name="mx" prefix-only="false"> | |||
| <suffix name="GroupBy" /> | |||
| <suffix name="Map" /> | |||
| <suffix name="RetainDetached" /> | |||
| <suffix name="RetainDetached" default="10" /> | |||
| <suffix name="Accept" /> | |||
| <suffix name="Reject" /> | |||
| </class> | |||
|
|
|||
| <section name="Ice"> | |||
There was a problem hiding this comment.
There is no default for the ThreadPool properties above?
There was a problem hiding this comment.
There are, I just haven't gotten to them yet. IIRC they don't read them "regularly". Will look closer at the ones that are currently unset in a follow-up PR.
| @@ -842,7 +842,7 @@ Glacier2::SessionFactoryHelper::createInitData() | |||
| // plug-in has already been setup we don't want to override the | |||
There was a problem hiding this comment.
Seems I forgot to remove this, when moving SSL transport into the core.
This PR starts the process of updating the code to use default properties. I specifically focused on C++ and primarily on functions that used
getProperty(AsXXX)WithDefault. I also replaced many calls togetPropertywhere we were fetching an Ice property or relying on the default of""or0.So, not all properties have defaults set yet. I set the defaults with the following logic:
0,100,Random. then I used that. If the property had no documented default, or if we documented that the default relies on other properties, the os, etc. then I did not set any explicit default value.There is still more to do after this: