smiklosovic commented on code in PR #4406:
URL: https://github.com/apache/cassandra/pull/4406#discussion_r2394445121
##########
src/java/org/apache/cassandra/config/GuardrailsOptions.java:
##########
@@ -1424,12 +1471,39 @@ private static Set<String>
validateTableProperties(Set<String> properties, Strin
Set<String> diff = Sets.difference(lowerCaseProperties,
TableAttributes.allKeywords());
if (!diff.isEmpty())
- throw new IllegalArgumentException(format("Invalid value for %s:
'%s' do not parse as valid table properties",
- name, diff));
+ throw new IllegalArgumentException(invalidValueMessage(name, diff,
"table"));
+
+ return lowerCaseProperties;
+ }
+
+ private static Set<String> validateKeyspaceProperties(Set<String>
properties, String name)
+ {
+ if (properties == null)
+ throw new IllegalArgumentException(format("Invalid value for %s:
null is not allowed", name));
+
+ Set<String> lowerCaseProperties =
properties.stream().map(LocalizeString::toLowerCaseLocalized).collect(toSet());
+
+ if (lowerCaseProperties.contains("replication"))
Review Comment:
ok hear me out, the last thing I want to mention before we ship this. First
of all, you can use `KeyspaceParams.Option.REPLICATION.toString()` here, and
secondly, which is more important, I think that we should introduce
`Set<String>` into `KeyspaceAttributes` called "requiredKeywords" which would
contain only `REPLICATION.toString()` for now. Then, we would do first diff to
see if any required are among `lowerCaseProperties` and errored out if they are.
The reason behind this is that whoever will introduce any new required
keyspace parameter into `KeyspaceAttributes` will not need to know about the
existence of this particular logic whatsoever. They just need to declare that
so and so keyspace parameter is required and everything would just work. (It is
ever harder if we just wire here "replication" string instead of reusing Option
so a caller can at least have a chance to manually see where these options are
used).
Otherwise I am +1. @maedhroz feel free to drive this to the completion and
merge, I will not be available until next week Monday-ish.
--
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]
