CASSANDRA-17797 All system properties and environment variables must be accessed via CassandraRelevantProperties

[Mmuzaf]

Thanks for sending a pull request! Here are some tips if you're new here:

• Ensure you have added or run the appropriate tests for your PR.
• Be sure to keep the PR description updated to reflect all changes.
• Write your PR title to summarize what this PR proposes.
• If possible, provide a concise example to reproduce the issue for a faster review.
• Read our contributor guidelines
• If you're making a documentation change, see our guide to documentation contribution

Commit messages should follow the following format:

<One sentence description, usually Jira title or CHANGES.txt summary>

<Optional lengthier description (context on patch)>

patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####

Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>

The Cassandra Jira

[jacek-lewandowski]

Great job. I left some comments, mostly minor suggestions.
One more general idea is to have all those get/set methods in an interface as defaults and inherit them in CassandraRelevantProperties and the env counterpart

[jacek-lewandowski]

why don't you add a method Optional<String> getStringOpt(), such snippets could be largely simplified, like:

return REPLACE_ADDRESS.getStringOpt()
                      .orElseGet(REPLACE_ADDRESS_FIRST_BOOT::getStringOpt)
                      .map(InetAddressAndPort::getByName)
                      .orElse(null);

[smiklosovic]

+1 if it does the same thing.

[jacek-lewandowski]

As a general note to this file, I think we should:

• remove redundant defaults, like null, false (maybe fail if such default is defined)
• maybe fail if a default is defined for non-cassandra property - can we really assume such default?
• perhaps have overloaded set() method rather than setString, setInt, setLong, etc.
• have additional methods to get as optional
• have methods to check if is unset or default
• maybe have some methods to get values as DataStorageSpec and DurationSpec objects
• have some static initializer which asserts there are no duplicates
• rename enums so that they reflect the area of application
• rename some pure test properties which lacks reasonable prefix
• sort items

[smiklosovic]

Some of these are relevant to implement but I would really focus on moving it all to CRP first and then we can optimize in other tickets. Or we may spend more time on this, sure ... depends how tasteful it will be for Maxim to chew through that.

[jacek-lewandowski]

I think those are simple modifications, which can be significantly aided with IDE

[Mmuzaf]

@jacek-lewandowski Thanks for the comments.

At the first glance, I think we can fix most of them. Please keep in mind that due to the number of changed files in the patch, any additional changes other than a simple move to CRP will increase the time it takes to review and make conflict resolution a bit more difficult while we are still waiting for the patch to be reviewed. So the idea (as I understand the goal) is only do moves :-)

For example, you're right about sorting enum names (newly added names are sorted), but not all of them in the class. My mistake.

One important thing I'd like to mention:

We can't move all the defaults to CRP, especially those that are get calculated e.g. Gossiper.intervalInMillis * 2 it may trigger unnecessary classloading due to static variables being used, but moving the simple constant values is good if it makes sense for the subsystem. So, checking for a 'default' value might be a bit confusing for the cases where this default value is not present in CRP.

[jacek-lewandowski]

@Mmuzaf regarding defaults - I agree. What I mean was actually that we should forbid defaults in CRP in two cases:

• this is not a Cassandra specific property - the rationale is that it can be misleading and hard to track - for example, imagine a netty property which is obviously defined in CRP. We have no idea what default the relevant component (like Netty) assumed. Moreover it may change with a new component version I doubt anybody will ever track that. In other words, non Cassandra-specific properties are defined in CRP to be set rather than read, so defaults for them are confusing. Maybe even reading from them should be forbidden? IDK
• the default value is the default default value :D, like null, false, 0

I don't think you need more reviews. If you put each IDE-aided refactoring in a distinct commit, we will not have to go through all the changes.

[jacek-lewandowski]

@Mmuzaf please don't force push and apply changes in separate commits; you will rebase once we finish the review phase

[michaelsembwever]

One more general idea is to have all those get/set methods in an interface as defaults and inherit them in CassandraRelevantProperties and the env counterpart

Doesn't this go against our code style. We don't add get/set methods if there's not a reason to do so, and we don't add interfaces unless there's a reason to do so.

[jacek-lewandowski]

One more general idea is to have all those get/set methods in an interface as defaults and inherit them in CassandraRelevantProperties and the env counterpart

Doesn't this go against our code style. We don't add get/set methods if there's not a reason to do so, and we don't add interfaces unless there's a reason to do so.

I'm not suggesting adding any new methods, just moving the existing methods to the interface because they can not go to a superclass (enums). The only difference between CassandraRelevantProperties and CassandraRelevantEnvs is the method they use to get the raw string. Sorry for the confusion - no set methods to be moved, just get.

[jacek-lewandowski]

Let's finish it, please just create follow-up tickets for the remaining stuff. Maybe create an epic and collect everything under it.

[jacek-lewandowski]

Committed: f650908