ctubbsii commented on code in PR #5886:
URL: https://github.com/apache/accumulo/pull/5886#discussion_r2357148935
##########
core/src/test/resources/accumulo3.properties:
##########
Review Comment:
I would give this file a more descriptive name, like
`accumulo-props-with-table.properties`; could also add some comments to the
file to explain what it should contain
##########
core/src/test/java/org/apache/accumulo/core/conf/SiteConfigurationTest.java:
##########
@@ -93,4 +94,16 @@ public void testConfigOverrides() {
conf.getProperties(results, p -> p.startsWith("instance"));
assertEquals("myhost:2181",
results.get(Property.INSTANCE_ZK_HOST.getKey()));
}
+
+ @Test
+ public void testTableProps() {
+ // try setting a table prop as an override
+ assertThrows(IllegalArgumentException.class, () ->
SiteConfiguration.empty()
+ .withOverrides(Map.of(Property.TABLE_MAJC_RATIO.getKey(),
"5")).build());
+
+ // try loading a properties file that has a a table prop
+ URL propsUrl =
getClass().getClassLoader().getResource("accumulo3.properties");
Review Comment:
With the file move/rename I suggested in the other comment, this becomes:
```suggestion
URL propsUrl =
getClass().getResource("SiteConfigurationTest-testTableProps.properties");
```
This could be further made dynamic using `getClass().getSimpleName()` or the
JUnit's features to get the test name (several tests extend `WithTestNames`,
which is a convenience class to make it easier to get the test name with less
boilerplate.
At the very least, putting it in the same path as the class, regardless of
the filename part, makes the getResource() much simpler, and you don't need to
add another exception to the check script's TODO section.
##########
core/src/main/java/org/apache/accumulo/core/conf/SiteConfiguration.java:
##########
@@ -204,6 +204,12 @@ public static SiteConfiguration auto() {
private SiteConfiguration(Map<String,String> config) {
ConfigCheckUtil.validate(config.entrySet(), "site config");
+ config.forEach((prop, value) -> {
+ if (prop.startsWith(Property.TABLE_PREFIX.getKey())) {
+ throw new IllegalArgumentException(
+ "Site configuration can not contain table properties, saw : " +
prop);
Review Comment:
```suggestion
"Site configuration must not contain table properties, saw : " +
prop);
```
##########
core/src/test/resources/accumulo2.properties:
##########
@@ -21,7 +21,6 @@ general.rpc.timeout=123s
instance.secret=mysecret
instance.volumes=hdfs://localhost:8020/accumulo123
instance.zookeeper.host=myhost123:2181
-table.durability=flush
Review Comment:
This looks safe. I don't think any test code was relying on this being
`flush` instead of the default, `sync`.
##########
core/src/main/java/org/apache/accumulo/core/conf/SiteConfiguration.java:
##########
@@ -204,6 +204,12 @@ public static SiteConfiguration auto() {
private SiteConfiguration(Map<String,String> config) {
ConfigCheckUtil.validate(config.entrySet(), "site config");
+ config.forEach((prop, value) -> {
Review Comment:
You could also use a stream collector to collect all of the violations to
include in a single exception, so users don't need to fix one, see another
error, fix another, etc.
##########
src/build/ci/check-module-package-conventions.sh:
##########
@@ -59,6 +59,7 @@ ALLOWED=(
core/src/test/resources/empty.jceks
core/src/test/resources/site-cfg.jceks
core/src/test/resources/accumulo2.properties
+ core/src/test/resources/accumulo3.properties
Review Comment:
This shouldn't be added to this directory, and shouldn't require a new
exception added to the existing TODO section of the file. Instead, it should be
in:
```
core/src/test/resources/org/apache/accumulo/core/
```
somewhere, preferably named after the test case, so maybe:
```
core/src/test/resources/org/apache/accumulo/core/conf/SiteConfigurationTest-testTableProps.properties
```
##########
core/src/test/java/org/apache/accumulo/core/spi/compaction/RatioBasedCompactionPlannerTest.java:
##########
@@ -791,7 +791,7 @@ public void testMaxTabletFilesNoCompaction() {
var overrides2 = new HashMap<>(overrides);
overrides2.put(Property.COMPACTION_SERVICE_PREFIX.getKey() +
"cs1.planner.opts.lowestRatio",
"1.04");
- var conf2 = new
ConfigurationImpl(SiteConfiguration.empty().withOverrides(overrides2).build());
+ var conf2 = new ConfigurationImpl(new ConfigurationCopy(overrides2));
Review Comment:
I think this part should be reverted. Whereas ConfigurationCopy is being
repurposed from being a cache of config properties to being a test copy of a
real AccumuloConfiguration instance, SiteConfiguration is explicitly a real
AccumuloConfiguration, with DefaultConfiguration as its parent. So, they have
different behavior.
I think we should generally avoid ConfigurationCopy unless we're really
using it as a temporary copy, like a cached snapshot, or if we really need
fine-grained control over its contents for testing the configuration code
itself. If we just want to emulate a realistic AccumuloConfiguration with some
specific values set by the user, to test some component of Accumulo, using
SiteConfiguration is better I think, because it's more realistic to to the
behavior of a real configuration object.
--
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]
