[
https://issues.apache.org/jira/browse/CASSANDRA-17797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17714247#comment-17714247
]
Maxim Muzafarov commented on CASSANDRA-17797:
---------------------------------------------
I am posting the same comments here because they are valuable to the issue
itself, and because the community has agreed that all such discussions need to
be within JIRA. The original comment is here:
[https://github.com/apache/cassandra/pull/2046#discussion_r1157582059]
----
Folks, I'd like to continue with this as I've found some inconsistencies in the
source code regarding the use of defaults. I'd like to discuss with you some
thoughts about the {{CassandraRelevantProperties}} default values. My concern
here is that the current API for getting the value of system properties doesn't
give us a reliable and explicit way to get the same results each time we access
a property, so removing "redundant" defaults may cause more problems than
adding them explicitly.
h3. Current state
Let's take a look at a few examples and inconsistencies:
# Inconsistency between {{getBoolean()}} and
{{{}getInt(){}}}/{{{}getLong(){}}} methods:
* in case {{getBoolean()}} without defaults value the {{false}} value
returned, no exception is thrown;
* in case {{getInt()}} or {{getLong()}} with default value the exception is
thrown ({{{}NullPointerException{}}} :-( see
[testInteger_null()|https://github.com/apache/cassandra/blob/trunk/test/unit/org/apache/cassandra/config/CassandraRelevantPropertiesTest.java#L133]
test);
# Inconsistency between {{getBoolean()}} and {{getString()}} for the same
property (assume a system property with type boolean is used):
* {{getBoolean()}} will return a non-null value if the default value is
{{{}null{}}};
* {{getString()}} for the same property will return a {{null}} value;
# If the default value is {{null}} the {{NullPointerException}} is thrown:
The {{getBoolean()}} method implicitly returns the {{false}} value and depends
on the boolean converter implementation itself (so a developer has to delve
into the code to find the _real_ return value). However, the {{public long
getLong()}} will throw {{NullPointerException}}
h3. Expected state
So, I think the API should be the following (default value {{null}} is allowed)
and we have to fix it:
# {{boolean disableSTCSInL0 = DISABLE_STCS_IN_L0.getBoolean(false);}} (runtime
option)
* If the property is set, the corresponding system property value is used;
* If the property is NOT set, the default value from brackets is used;
_OR_
# {{DISABLE_STCS_IN_L0("cassandra.disable_stcs_in_l0", "false")}} (compile
time option)
* If the property is set, the corresponding system property value is used;
* If the property is NOT set, the default value from the constant is used;
_OR_
# {{boolean disableSTCSInL0 = DISABLE_STCS_IN_L0.getBoolean()}} (property is
expected to be set)
* If the property is set, the corresponding system property value is used;
* If the property is NOT set, and there is NO default value we MUST throw an
exception.
WDYT?
> All system properties and environment variables should be accessed via the
> new CassandraRelevantProperties and CassandraRelevantEnv classes
> -------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: CASSANDRA-17797
> URL: https://issues.apache.org/jira/browse/CASSANDRA-17797
> Project: Cassandra
> Issue Type: Improvement
> Components: Local/Config
> Reporter: Ekaterina Dimitrova
> Assignee: Maxim Muzafarov
> Priority: Low
> Fix For: 5.x
>
> Time Spent: 32h 40m
> Remaining Estimate: 0h
>
> Follow up ticket for CASSANDRA-15876 -
> "Always access system properties and environment variables via the new
> CassandraRelevantProperties and CassandraRelevantEnv classes"
> As part of that ticket we moved to the two new classes only
> properties/variables that were currently listed in System Properties Virtual
> Table.
> We have to move to those classes the rest of the properties around the code
> and start using those classes to access all of them.
> +Additional information for newcomers:+
> You might want to start by getting acquainted with
> CassandraRelevantProperties and CassandraRelevantEnv classes. Also, you might
> want to check what changes were done and how the first batch was transferred
> to this new framework as part of
> [CASSANDRA-15876|https://github.com/apache/cassandra/commit/7694c1d191531ac152db55e83bc0db6864a5441e]
> We are interested into the properties accessed currently through
> getProperties around the code.
> As part of CASSANDRA-15876 relevant unit tests were added
> (CassandraRelevantPropertiesTest). To verify the new patch we need to ensure
> that all tests Cassandra pass and also to think about potential update of the
> mentioned test class.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
