Hi Maxim, thanks for bringing this up. I am glad you did the heavy-lifting in / around CassandraRelevantProperties and we can build on top of this.
I am fine with @Replaces for Cassandra system properties. After we put everything into CassandraRelevantProperties, one can easily see that there are great inconsistencies in properties' naming. As we still need to support the old names too, using @Replaces, the similar mechanism we used in DatabaseDescriptor, seems like the ideal solution. By the way, when somebody queries system_views.system_properties, it looks very strange in CQL shell, the formatting is just broken. EXPAND ON; does not help either. It is quite hard to parse this visually if a user wants to see them all. The reason is that there is "java.class.path" property for which the value is so long that it basically breaks the output. Another solution would be to fix the output but I am not sure how it would look like. As we are going to rename them to have same prefixes, could not we remodel that table as well? For example: https://gist.github.com/smiklosovic/de662b7faa25e1fdd56805cdb5ba80a7 Feel free to come up with a different approach. By doing this, it would be way easier to get just Cassandra properties or just properties for tests or just Java properties and selecting just the first two groups would not break CQLSH. It is nice that it would have same prefix but I am trying to find a way how to utilize the same prefix in CQLSH as well. ________________________________________ From: Maxim Muzafarov <mmu...@apache.org> Sent: Thursday, May 18, 2023 12:54 To: dev@cassandra.apache.org Subject: Re: [DISCUSS] Moving system property names to the CassandraRelevantProperties NetApp Security WARNING: This is an external email. Do not click links or open attachments unless you recognize the sender and know the content is safe. Hello everyone, Thanks for following this thread and the review, all the system properties have been moved to CassandraRelevantProperties. So you can find out what it looks like from the following link: https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java#L38 I would like to show you a few more steps in this thread so that the solution is generally complete. As you may have noticed, we have three types of system properties: cassandra properties used in production environments, cassandra properties used for testing only, and non-cassandra properties. I would like to reuse the @Replaces annotation to rename cassandra-related properties according to the following pattern: the 'cassandra.' prefix is for production properties, and the 'cassandra.test' prefix is for testing properties. This makes the results of the SystemPropertiesTable virtual table look more natural to users. I thinks we should include this change for the 5.0 release. WDYT? The other code clarity minor improvements to do: 1. Use WithProperties to ensure that system properties are handled https://issues.apache.org/jira/browse/CASSANDRA-18453 2. As a draft agreement, the CassandraRelevantProperties and CassandraRelevantEnv (and probably DatabaseDescriptor) could share the same interface to access the system properties, configuration properties, and/or environment variables. The idea is still in draft form, so I'm mentioning it here to keep it in context. Will come back to it when more details are available. This is what it might look like: https://github.com/apache/cassandra/pull/2300/files#diff-6b7db8438314143a1b6b1c8c58901a4ebbbb3954af8cdd294ca8853a1001c1f4R70 On Fri, 31 Mar 2023 at 07:08, Jacek Lewandowski <lewandowski.ja...@gmail.com> wrote: > > I'll do > > - - -- --- ----- -------- ------------- > Jacek Lewandowski > > > czw., 30 mar 2023 o 22:09 Miklosovic, Stefan <stefan.mikloso...@netapp.com> > napisał(a): >> >> Hi list, >> >> we are looking for one more committer to take a look at this patch (1, 2). >> >> It looks like there is a lot to go through because of number of files >> modified (around 200) but changes are really just about moving everything to >> CassandraRelevantProperties. I do not think that it should take more than 1 >> hour of dedicated effort and we are done! >> >> Thanks in advance to whoever reviews this. >> >> I want to especially thank Maxim for his perseverance in this matter and I >> hope we will eventually deliver this work to trunk. >> >> (1) https://github.com/apache/cassandra/pull/2046 >> (2) https://issues.apache.org/jira/browse/CASSANDRA-17797 >> >> Regards >> >> Regards >> >> ________________________________________ >> From: Miklosovic, Stefan <stefan.mikloso...@netapp.com> >> Sent: Wednesday, March 22, 2023 14:34 >> To: dev@cassandra.apache.org >> Subject: Re: [DISCUSS] Moving system property names to the >> CassandraRelevantProperties >> >> NetApp Security WARNING: This is an external email. Do not click links or >> open attachments unless you recognize the sender and know the content is >> safe. >> >> >> >> >> Hi Maxim, >> >> thanks for letting us know. >> >> I reviewed it couple months ago but I can revisit it to double check. We >> need the second reviewer. Until we find somebody, we can not merge this. >> >> If anybody wants to take a look, it would be awesome. It seems like a lot of >> changes / files touched but it is just about centralizing all properties >> scattered around the code base into one place. >> >> Regards >> >> ________________________________________ >> From: Maxim Muzafarov <mmu...@apache.org> >> Sent: Tuesday, March 21, 2023 22:59 >> To: dev@cassandra.apache.org >> Subject: Re: [DISCUSS] Moving system property names to the >> CassandraRelevantProperties >> >> NetApp Security WARNING: This is an external email. Do not click links or >> open attachments unless you recognize the sender and know the content is >> safe. >> >> >> >> >> Hello everyone, >> >> This a friendly reminder that some help is still needed with the review :-) >> I have resolved all the conflicts that have arisen in the last month or two. >> >> If you'd like to invest some time in code clarity, please take a look: >> https://github.com/apache/cassandra/pull/2046/files >> >> On Wed, 8 Feb 2023 at 19:48, Maxim Muzafarov <mmu...@apache.org> wrote: >> > >> > Hello everyone, >> > >> > >> > We are trying to clean up the source code around the direct use of >> > system properties and make this use more manageable and transparent. >> > To achieve this, I have prepared a patch that moves all system >> > property names to the CassandraRelevantProperties, which in turn makes >> > some of the properties visible to a user through the >> > SystemPropertiesTable virtual table. >> > >> > The patch has passed a few rounds of review, but we still need another >> > pair of eyes to make sure we are not missing anything valuable. >> > Please, take a look at the patch. >> > >> > You can find all the changes here: >> > https://issues.apache.org/jira/browse/CASSANDRA-17797 >> > >> > >> > I'd also like to share the names of the properties that will appear in >> > the SystemPropertiesTable, the appearance of which is related to the >> > public API changes we agreed to discuss on the dev list. >> > >> > >> > The public API changes >> > >> > Newly production system properties added: >> > >> > io.netty.eventLoopThreads >> > io.netty.transport.estimateSizeOnSubmit >> > java.security.auth.login.config >> > javax.rmi.ssl.client.enabledCipherSuites >> > javax.rmi.ssl.client.enabledProtocols >> > ssl.enable >> > log4j2.disable.jmx >> > log4j2.shutdownHookEnabled >> > logback.configurationFile >> > >> > Newly added and used for tests only: >> > >> > invalid-legacy-sstable-root >> > legacy-sstable-root >> > org.apache.cassandra.tools.UtilALLOW_TOOL_REINIT_FOR_TEST >> > org.caffinitas.ohc.segmentCount >> > suitename >> > sun.stderr.encoding >> > sun.stdout.encoding >> > test.bbfailhelper.enabled >> > write_survey
