Mmuzaf commented on code in PR #2046: URL: https://github.com/apache/cassandra/pull/2046#discussion_r1158370862
########## src/java/org/apache/cassandra/config/CassandraRelevantProperties.java: ########## Review Comment: @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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
