smiklosovic commented on code in PR #4242: URL: https://github.com/apache/cassandra/pull/4242#discussion_r2207308019
########## src/java/org/apache/cassandra/tools/nodetool/GuardrailsConfigCommand.java: ########## @@ -0,0 +1,445 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.tools.nodetool; + +import java.lang.reflect.Method; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.function.Function; +import java.util.regex.Pattern; +import java.util.stream.Stream; + +import io.airlift.airline.Arguments; +import io.airlift.airline.Command; +import io.airlift.airline.Option; +import org.apache.cassandra.db.guardrails.GuardrailsMBean; +import org.apache.cassandra.tools.NodeProbe; +import org.apache.cassandra.tools.NodeTool; +import org.apache.cassandra.tools.nodetool.formatter.TableBuilder; +import org.apache.cassandra.utils.LocalizeString; + +import static java.lang.String.format; +import static java.util.Arrays.stream; +import static java.util.Comparator.comparing; +import static java.util.stream.Collectors.toList; + +public abstract class GuardrailsConfigCommand extends NodeTool.NodeToolCmd +{ + @Command(name = "getguardrailsconfig", description = "Print current guardrails configuration.") + public static class GetGuardrailsConfig extends GuardrailsConfigCommand + { + @Option(name = { "--category", "-c" }, + description = "Category of guardrails to filter, can be one of 'values', 'thresholds', 'flags', 'other'") + private String guardrailCategory; + + @Arguments(description = "Specific names or guardrails to get configuration of.") + private List<String> args = new ArrayList<>(); + + @Override + public void execute(NodeProbe probe) + { + if (!args.isEmpty() && guardrailCategory != null) + throw new IllegalStateException("Do not specify additional arguments when --category/-c is set."); + + validateCategory(guardrailCategory); + + List<Method> allGetters = stream(probe.getGuardrailsMBean().getClass().getDeclaredMethods()) + .filter(method -> method.getName().startsWith("get") Review Comment: This is a good question and I was battling with it myself. In this PR I was just following the approach in the original PR. Another option is to enumerate absolutely every guardrail and map it to getter / setter in MBean. Basically a monstrous map of guardrail name as key and getters /setters as a value. How it is done currently does require specific naming but on the other hand it seems to me that we are already following this convention subconsciously and even if we diverged from that upon an implementation of yet another guardrail it would be detected in tests. If there is a guardrail added and respective MBean methods added as well, it would be automatically returned when parsed and then it would appear in the output and it would fail the test because there would not be such an entry in expected test data. That would be the chance to see that naming is wrong / strange and a developer would know that MBean method name needs to be accommodated. On the other hand, if we implemented "big map" and an implementator forgot it to add to that map, then because it is not parsed like currently is, all tests would just pass because ... well ... nodetool output has not changed. Based on the naming convention of get / set methods in MBean I think it is pretty safe to assume that we will be indeed adding get/set prefix to every method. It is the camel part after it which translates to snake which needs attention a bit. Even if, in an improbable case of a developer naming it wrong (as I did with zero_ttl_on_twcs_enabled MBean methods), there is the translation map to cover such special cases. We can not just rename a MBean method when we detect its naming is not right / ideal because MBean interface is a part of contract we committed ourselves into and it would need to go through class deprecation lifecycle. Even then we would not remove it, we would most probably just add a new method with correct naming. I was also exploring if it is possible to put `@Deprecated` annotations on MBean interface and parse this in nodetool so we would skip such methods and used their replacements but for some reason JMX does not return annotations for the methods when accessed via reflection. waiting for @driftx to see what his opinion is -- 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: pr-unsubscr...@cassandra.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: pr-unsubscr...@cassandra.apache.org For additional commands, e-mail: pr-h...@cassandra.apache.org
