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 just loop over
given properties and see if it is in required parameters and error out if it is.
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
even 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).
It is way more probable and more developer-friendly if a developer just adds
an option to required ones as it is being added, instead of him / her trying to
figure out what logic there is in the whole codebase he / she needs to
incorporate this new parameter into.
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.
Using `KeyspaceParams.Option.REPLICATION.toString()` is required from my
side to merge this. Using dedicated requiredKeywords as I suggested is strongly
recommended but not a blocker.
--
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]
