keith-turner commented on code in PR #4092:
URL: https://github.com/apache/accumulo/pull/4092#discussion_r1434607989
##########
core/src/main/java/org/apache/accumulo/core/conf/ConfigurationCopy.java:
##########
@@ -30,6 +30,8 @@
* configuration
*/
public class ConfigurationCopy extends AccumuloConfiguration {
+
+ AccumuloConfiguration parent = null;
Review Comment:
Would be good to do something similar to this for isSet. Like the way it
all checks the parent. I don't think it matters for this PR, but would make
the use parent consistent in case this is used by other stuff.
https://github.com/apache/accumulo/blob/2890cc2c99a1ae0fd1531533df6ee75f8b2f1cfa/core/src/main/java/org/apache/accumulo/core/conf/SiteConfiguration.java#L249
##########
core/src/test/java/org/apache/accumulo/core/spi/compaction/DefaultCompactionPlannerTest.java:
##########
@@ -152,12 +171,126 @@ public void testRunningCompaction() {
// planner should compact.
var job = getOnlyElement(plan.getJobs());
assertEquals(candidates, job.getFiles());
- assertEquals(CompactionExecutorIdImpl.externalId("medium"),
job.getExecutor());
+ assertEquals(CompactionExecutorIdImpl.internalId(csid, "medium"),
job.getExecutor());
+ }
+
+ /**
+ * Tests that the maxOpen property overrides the deprecated open.max
property with the default
+ * service
+ */
+ @Test
+ @SuppressWarnings("removal")
+ public void testOverrideMaxOpenDefaultService() {
+ // Set old property and use that for max open files.
+ ConfigurationCopy aconf = new ConfigurationCopy(Map.of(),
DefaultConfiguration.getInstance());
+ aconf.set(Property.TSERV_MAJC_THREAD_MAXOPEN.getKey(), "17");
+ ConfigurationImpl config = new ConfigurationImpl(aconf);
+
+ ServiceEnvironment senv = EasyMock.createMock(ServiceEnvironment.class);
+ EasyMock.expect(senv.getConfiguration()).andReturn(config).anyTimes();
+ EasyMock.replay(senv);
+
+ // Use the CompactionServicesConfig to create options based on default
property values
+ var CompactionServices = new CompactionServicesConfig(aconf, log::warn);
+ var options = CompactionServices.getOptions().get("default");
+
+ var initParams =
+ new CompactionPlannerInitParams(CompactionServiceId.of("default"),
options, senv);
+
+ var planner = new DefaultCompactionPlanner();
+ planner.init(initParams);
+
+ var all = createCFs("F1", "10M", "F2", "11M", "F3", "12M", "F4", "13M",
"F5", "14M", "F6",
+ "15M", "F7", "16M", "F8", "17M", "F9", "18M", "FA", "19M", "FB",
"20M", "FC", "21M", "FD",
+ "22M", "FE", "23M", "FF", "24M", "FG", "25M", "FH", "26M");
+ Set<CompactionJob> compacting = Set.of();
+ var params = createPlanningParams(all, all, compacting, 2,
CompactionKind.USER);
+ var plan = planner.makePlan(params);
+ var job = getOnlyElement(plan.getJobs());
+ assertEquals(all, job.getFiles());
+
assertEquals(CompactionExecutorIdImpl.internalId(CompactionServiceId.of("default"),
"large"),
+ job.getExecutor());
+
+ aconf.set(Property.TSERV_MAJC_THREAD_MAXOPEN.getKey(), "5");
+ // Create new initParams so executor IDs can be reused
+ initParams = new
CompactionPlannerInitParams(CompactionServiceId.of("default"), options, senv);
+ planner = new DefaultCompactionPlanner();
+ planner.init(initParams);
+
+ params = createPlanningParams(all, all, compacting, 2,
CompactionKind.USER);
+ plan = planner.makePlan(params);
+ job = getOnlyElement(plan.getJobs());
+ assertEquals(createCFs("F1", "10M", "F2", "11M", "F3", "12M", "F4", "13M",
"F5", "14M"),
+ job.getFiles());
+
assertEquals(CompactionExecutorIdImpl.internalId(CompactionServiceId.of("default"),
"medium"),
+ job.getExecutor());
+ }
+
+ /**
+ * Tests that the maxOpen property overrides the deprecated open.max property
+ */
+ @Test
+ @SuppressWarnings("removal")
+ public void testOverrideMaxOpen() {
+ ConfigurationCopy aconf = new
ConfigurationCopy(DefaultConfiguration.getInstance());
Review Comment:
Can make the default config the parent which make the test a bit more like
the real env.
```suggestion
ConfigurationCopy aconf = new ConfigurationCopy(Map.of(),
DefaultConfiguration.getInstance());
```
##########
core/src/test/java/org/apache/accumulo/core/spi/compaction/DefaultCompactionPlannerTest.java:
##########
@@ -152,12 +171,126 @@ public void testRunningCompaction() {
// planner should compact.
var job = getOnlyElement(plan.getJobs());
assertEquals(candidates, job.getFiles());
- assertEquals(CompactionExecutorIdImpl.externalId("medium"),
job.getExecutor());
+ assertEquals(CompactionExecutorIdImpl.internalId(csid, "medium"),
job.getExecutor());
+ }
+
+ /**
+ * Tests that the maxOpen property overrides the deprecated open.max
property with the default
+ * service
+ */
+ @Test
+ @SuppressWarnings("removal")
+ public void testOverrideMaxOpenDefaultService() {
+ // Set old property and use that for max open files.
+ ConfigurationCopy aconf = new ConfigurationCopy(Map.of(),
DefaultConfiguration.getInstance());
+ aconf.set(Property.TSERV_MAJC_THREAD_MAXOPEN.getKey(), "17");
+ ConfigurationImpl config = new ConfigurationImpl(aconf);
+
+ ServiceEnvironment senv = EasyMock.createMock(ServiceEnvironment.class);
+ EasyMock.expect(senv.getConfiguration()).andReturn(config).anyTimes();
+ EasyMock.replay(senv);
+
+ // Use the CompactionServicesConfig to create options based on default
property values
+ var CompactionServices = new CompactionServicesConfig(aconf, log::warn);
Review Comment:
This variable name was confusing me, I thought it was a class name later and
thought something was calling static method on a class.
```suggestion
var compactionServices = new CompactionServicesConfig(aconf, log::warn);
```
--
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]
