ctubbsii commented on code in PR #5886:
URL: https://github.com/apache/accumulo/pull/5886#discussion_r2361358388
##########
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:
It was surprising to me that there was a table property in there. So I dug
into the code a bit more. It looks like the tests are mocking two different
ServiceEnvironments. The first one is for `.init(InitParameters)`, and the
other is for `planner.makePlan(PlanningParameters)`. Only the system/site
configuration is used for `init`, but the TableConfiguration is used for the
`makePlan`, so the `ServiceEnvironment.getConfiguration(TableId)` is only
mocked in the second ServiceEnvironment, and the one used for `init` only mocks
`ServiceEnvironment.getConfiguration()` (without a TableId).
This is very confusing because I generally expect the same
`ServiceEnvironment` to be used for both, and I was further confused because I
didn't understand that the table property was going into the mock object for
the system configuration, where it shouldn't be used from. However, I now
realize that this is the case because the same config is being used to back
both mocked `ServiceEnvironment` instances.
This code should be improved (in a different PR) to do separate the config
that is used for mocking the system properties from the config that is used for
mocking the table properties... something like:
```java
var mockServiceEnvironment = createMockServEnv(systemConfigOverrides,
tableConfigOverrides);
var mockInitParams = createMockInitParams(mockServiceEnvironment);
var mockPlanningParams = createMockPlanningParams(mockServiceEnvironment);
```
That would make it clear that that the table prop overrides aren't being
used in the system config, and would allow the resulting `ServiceEnvironment`
to be reused, but the tableConfigOverrides would only be used for mocking
`ServiceEnvironment.getConfiguration(TableId)`, and not the method without a
TableId.
Having traversed that rabbit hole, I also discovered that elsewhere in the
test, we construct the ServiceEnvironment configuration differently, and that
can be done here the same way:
```suggestion
var conf2 = ServiceEnvironment.Configuration.from(overrides2, true);
```
This should give equivalent behavior, with the DefaultConfiguration elements
included, though the test still passes if `false` is used to exclude the
defaults. I guess none of the code being tested reads any of the properties
other than those set in the overrides, so the defaults don't matter in this
case.
I think my suggested line here should be used instead of either of the
previous two versions... since it correctly models the site/system
configuration, backed by defaults, for the InitParameters, but does so more
directly, rather than messing with the ConfigurationImpl constructor and
ConfigurationCopy.
Note: to illustrate what I was talking about, regarding the same config
carrying properties for both the InitParameters (which reads the system config)
and the PlanningParameters (which reads the table config), consider the line
two lines after this one, where `createPlanningParams` is called. It is using
`conf` instead of the modified `conf2` properties, and the reason why this
still works is because we modified the `lowestRatio` property, which apparently
affects the `InitParameters`, but is ignored by the `PlanningParameters`. But,
it's confusing that we modified props and then kept using the old `conf` that
wasn't modified and it still worked. That's because both the system props and
the table props are being carried in the same bucket. It would be much more
clear if we just passed the same mocked ServiceEnvironment as I explained above.
--
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]
