Mmuzaf commented on code in PR #4263:
URL: https://github.com/apache/cassandra/pull/4263#discussion_r2223827296
##########
src/java/org/apache/cassandra/tools/nodetool/GuardrailsConfigCommand.java:
##########
@@ -112,89 +175,96 @@ public static class SetGuardrailsConfig extends
GuardrailsConfigCommand
{
private static final Pattern SETTER_PATTERN = Pattern.compile("^set");
- @Option(name = { "--list", "-l" },
- description = "List all available guardrails setters")
- private boolean list;
-
- @Option(name = { "--category", "-c" },
- description = "Category of guardrails to filter, can be one of
'values', 'thresholds', 'flags', 'others'.",
- allowedValues = { "values", "thresholds", "flags", "others" })
- private String guardrailCategory;
-
@Arguments(usage = "[<setter> <value1> ...]",
description = "For flags, possible values are 'true' or 'false'. " +
- "For thresholds, two values are expected, first for
warning, second for failure. " +
- "For values, one value is expected, multiple values
separated by comma.")
+ "For thresholds, two values are expected, first for
failure, second for warning. " +
+ "For values, enumeration of values expected or one value
where multiple items are separated by comma. " +
+ "Setting for thresholds accepting strings and value
guardrails are reset by specifying 'null' value. " +
+ "For thresholds accepting integers, the reset value is
-1.")
private final List<String> args = new ArrayList<>();
@Override
public void execute(NodeProbe probe)
{
- if (!list && guardrailCategory != null)
- throw new IllegalStateException("--category/-c can be
specified only together with --list/-l");
+ if (args.isEmpty())
+ throw new IllegalStateException("No arguments.");
- GuardrailCategory categoryEnum =
GuardrailCategory.parseCategory(guardrailCategory, probe.output().out);
+ String snakeCaseName = args.get(0);
- if (args.isEmpty() && !list)
- throw new IllegalStateException("No arguments.");
+ Method setter = getAllSetters(probe).entrySet().stream()
+ .findFirst()
+ .map(o ->
o.getValue().get(0))
+ .orElseThrow(() -> new
IllegalStateException(format("Guardrail %s not found.", snakeCaseName)));
- if (list)
- display(probe, getAllSetters(probe), categoryEnum);
- else
- executeSetter(probe);
+ sanitizeArguments(setter, args);
+ validateArguments(setter, snakeCaseName, args);
+
+ List<String> methodArgs = args.subList(1, args.size());
+ try
+ {
+ setter.invoke(probe.getGuardrailsMBean(),
prepareArguments(methodArgs, setter));
+ }
+ catch (Exception ex)
+ {
+ String reason;
+ if (ex.getCause() != null && ex.getCause().getMessage() !=
null)
+ reason = ex.getCause().getMessage();
+ else
+ reason = ex.getMessage();
+
+ throw new IllegalStateException(format("Error occured when
setting the config for setter %s with arguments %s: %s",
+ snakeCaseName,
methodArgs, reason));
+ }
}
@Override
- public void addRow(List<InternalRow> bucket, GuardrailsMBean mBean,
Method method, String guardrailName) throws Throwable
+ public void addRow(List<InternalRow> bucket, GuardrailsMBean mBean,
List<Method> methods, String guardrailName) throws Throwable
{
- if (method.getParameterTypes().length == 1)
- constructRow(bucket, sanitizeSetterName(method),
method.getParameterTypes()[0].getName());
- else
- constructRow(bucket, sanitizeSetterName(method),
stream(method.getParameterTypes()).map(Class::getName).collect(toList()).toString());
+ if (methods.size() == 1)
+ {
+ Method method = methods.get(0);
+ if (method.getParameterTypes().length == 1)
+ constructRow(bucket, sanitizeSetterName(method),
method.getParameterTypes()[0].getName());
+ else
+ constructRow(bucket, sanitizeSetterName(method),
stream(method.getParameterTypes()).map(Class::getName).collect(toList()).toString());
+ }
}
- private List<Method> getAllSetters(NodeProbe probe)
+ private Map<String, List<Method>> getAllSetters(NodeProbe probe)
{
return
stream(probe.getGuardrailsMBean().getClass().getDeclaredMethods())
.filter(method -> method.getName().startsWith("set") &&
!method.getName().endsWith("CSV"))
.filter(method -> args.isEmpty() ||
args.contains(toSnakeCase(method.getName().substring(3))))
.sorted(comparing(Method::getName))
- .collect(toList());
+ .collect(Collectors.groupingBy(method ->
toSnakeCase(method.getName().substring(3))))
+ .entrySet()
+ .stream()
+ .sorted(Map.Entry.comparingByKey())
+ .collect(Collectors.toMap(Map.Entry::getKey,
+ Map.Entry::getValue,
+ (e1, e2) -> e1,
+ LinkedHashMap::new));
}
private String sanitizeSetterName(Method setter)
{
return
toSnakeCase(SETTER_PATTERN.matcher(setter.getName()).replaceAll(""));
}
- private void executeSetter(NodeProbe nodeProbe)
+ private void sanitizeArguments(Method setter, List<String> args)
{
- String snakeCaseName = args.get(0);
- String setterName = toCamelCase(args.get(0).startsWith("set_") ?
args.get(0) : "set_" + args.get(0));
-
- Method setter = getAllSetters(nodeProbe).stream()
- .findFirst()
- .orElseThrow(() -> new
IllegalStateException(format("Setter method %s not found. " +
-
"Run nodetool setguardrailsconfig --list " +
-
"to see available setters", setterName)));
-
- validateArguments(setter, snakeCaseName, args);
-
- List<String> methodArgs = args.subList(1, args.size());
- try
+ Class<?>[] parameterTypes = setter.getParameterTypes();
+ if (parameterTypes.length == 1 && parameterTypes[0] == Set.class)
{
- setter.invoke(nodeProbe.getGuardrailsMBean(),
prepareArguments(methodArgs, setter));
- }
- catch (Exception ex)
- {
- String reason;
- if (ex.getCause() != null && ex.getCause().getMessage() !=
null)
- reason = ex.getCause().getMessage();
- else
- reason = ex.getMessage();
-
- throw new IllegalStateException(format("Error occured when
setting the config for setter %s with arguments %s: %s",
- snakeCaseName,
methodArgs, reason));
+ if (args.size() > 2)
+ {
+ String guardrail = args.get(0);
+ // replace multiple arguments with one which is separated
by a single comma
+ String collectedArguments = String.join(",",
args.subList(1, args.size()));
+ args.clear();
Review Comment:
`args.clear();` - this could cause problems for someone in the future who
wants to debug the command because the input has magically changed inside one
of the methods, making it difficult to find (and read). I would suggest using
another array, leaving the input array as is or assing `guardrailName` and/or
`guardrailValue` to a different variables.
Anyway this is a nit.
--
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]
