David, > Should” we detect this was violated and fail the build?
You are asking a good question! Sure, a new checkstyle rule was added to address this case for production and test classes. On Thu, 9 Feb 2023 at 19:40, David Capwell <dcapw...@apple.com> wrote: > > All properties meant to be used only for tests would have a prefix like > "cassandra.test.name.of.property" and production properties would be > "cassandra.xyx". Once this is done, we can filter them out in vtable so there > would not be any test-related properties in production. Test properties > should be visible only when developing / testing Cassandra, in my opinion. > > > Good point, I wouldn’t want to expose properties meant to break C* for > testing… that implies to users we should be using it! > > I understand that there is a lot of legacy in place and we can not rename > properties just like that for people. > > > We could always look to do things like we did in Config (which you call out), > add a way to “migrate” to the new naming/format. I am not sure if there is > enough configs to justify this, but with 5.0 happening it may be a good time > to think about normalizing these. > > We are trying to clean up the source code around the direct use of system > properties and make this use more manageable and transparent. > > > I didn’t review, but one question I have is inline with "I would like to > describe the ideal state / end goal”, but from the point of view of > maintaining things…. If someone adds a new system property “should” they use > this enum? “Should” we detect this was violated and fail the build? > > If we migrate now, nothing stops us from adding new, causing someone else to > be forced to migrate after…. Its one of the issues with the current enum, > once it was created authors didn’t add to it always causing patches like this > to try to migrate… > > I am not trying to block your patch, if you don’t deal with this I am +0… > just saying that maintaince is something we must think about > > On Feb 9, 2023, at 6:37 AM, Miklosovic, Stefan <stefan.mikloso...@netapp.com> > wrote: > > Hi Maxim, > > I would like to describe the ideal state / end goal, from my perspective. > > All properties meant to be used only for tests would have a prefix like > "cassandra.test.name.of.property" and production properties would be > "cassandra.xyx". Once this is done, we can filter them out in vtable so there > would not be any test-related properties in production. Test properties > should be visible only when developing / testing Cassandra, in my opinion. > > All other system properties should also have some consistent naming in place. > > I understand that there is a lot of legacy in place and we can not rename > properties just like that for people. > > The approach I like is what was done to properties in cassandra.yaml. There > is @Replaces annotation put on properties in Config which enables users to > still use the old names. > > I can imagine that something like this would used here. If an old name is > specified, it would internally translate to a new name and only new names > would be returned by vtable. There might be also a column for old names so > people would know what new property the old one translates to and we should > also emit warning for users that the system properties they are using are in > the old format and they should move to the new ones. > > Anyway, I am glad this is happening and we are making progress. It will be > also way easier to dump all properties to the website when everything is > centralized at once place. > > Regards > > ________________________________________ > From: Maxim Muzafarov <mmu...@apache.org> > Sent: Wednesday, February 8, 2023 19:48 > To: dev@cassandra.apache.org > Subject: [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, > > > 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 > >
