keith-turner commented on code in PR #4083:
URL: https://github.com/apache/accumulo/pull/4083#discussion_r1430721101
##########
core/src/main/java/org/apache/accumulo/core/spi/compaction/DefaultCompactionPlanner.java:
##########
@@ -57,85 +57,47 @@
* compaction service you are configuring.
*
* <ul>
- * <li>{@code compaction.service.<service>.opts.executors} This is a json
array of objects where
- * each object has the fields:
+ * <li>Note that the CompactionCoordinator and at least one Compactor for
"Large" must be running.
+ * <li>{@code compaction.service.<service>.opts.maxOpen} This determines the
maximum number of files
+ * that will be included in a single compaction.
+ * <li>{@code compaction.service.<service>.opts.groups} This is a json array
of group objects which
+ * have the following fields:
* <table>
- * <caption>Default Compaction Planner Executor options</caption>
+ * <caption>Default Compaction Planner Queue options</caption>
* <tr>
* <th>Field Name</th>
* <th>Description</th>
* </tr>
* <tr>
* <td>name</td>
- * <td>name or alias of the executor (required)</td>
- * </tr>
- * <tr>
- * <td>type</td>
- * <td>valid values 'internal' or 'external' (required)</td>
+ * <td>name or alias of the group (required)</td>
* </tr>
* <tr>
* <td>maxSize</td>
* <td>threshold sum of the input files (required for all but one of the
configs)</td>
* </tr>
- * <tr>
- * <td>numThreads</td>
- * <td>number of threads for this executor configuration (required for
'internal', cannot be
- * specified for 'external')</td>
- * </tr>
- * <tr>
- * <td>group</td>
- * <td>name of the external compaction group (required for 'external', cannot
be specified for
- * 'internal')</td>
- * </tr>
* </table>
* <br>
- * Note: The "executors" option has been deprecated in 3.1 and will be removed
in a future release.
- * This example uses the new `compaction.service` prefix. The property prefix
- * "tserver.compaction.major.service" has also been deprecated in 3.1 and will
be removed in a
- * future release. The maxSize field determines the maximum size of compaction
that will run on an
- * executor. The maxSize field can have a suffix of K,M,G for kilobytes,
megabytes, or gigabytes and
- * represents the sum of the input files for a given compaction. One executor
can have no max size
- * and it will run everything that is too large for the other executors. If
all executors have a max
- * size, then system compactions will only run for compactions smaller than
the largest max size.
- * User, chop, and selector compactions will always run, even if there is no
executor for their
- * size. These compactions will run on the executor with the largest max size.
The following example
- * value for this property will create 3 threads to run compactions of files
whose file size sum is
- * less than 100M, 3 threads to run compactions of files whose file size sum
is less than 500M, and
- * run all other compactions on Compactors configured to run compactions for
Queue1:
+ * This 'groups' object is used for defining compaction groups. The maxSize
field determines the
+ * maximum size of compaction that will run in a group. The maxSize field can
have a suffix of K,M,G
+ * for kilobytes, megabytes, or gigabytes and represents the sum of the input
files for a given
+ * compaction. One group can have no max size and it will run everything that
is too large for the
+ * other groups. If all groups have a max size, then system compactions will
only run for
+ * compactions smaller than the largest max size. User, chop, and selector
compactions will always
Review Comment:
This should have already been removed, but it looks like we missed it. Chop
compactions are gone, so can drop chop.
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -53,12 +53,40 @@ public enum Property {
+ "A new external compaction service would be defined like the
following:\n"
+ "`compaction.service.newService.planner="
+
"\"org.apache.accumulo.core.spi.compaction.DefaultCompactionPlanner\".`\n"
- + "`compaction.service.newService.opts.queues=\""
+ + "`compaction.service.newService.opts.groups=\""
+ "[{\"name\": \"small\", \"maxSize\":\"32M\"},"
+ "{ \"name\":\"medium\",
\"maxSize\":\"512M\"},{\"name\":\"large\"}]`\n"
+ "`compaction.service.newService.opts.maxOpen=50`.\n"
+ "Additional options can be defined using the
`compaction.service.<service>.opts.<option>` property.",
"3.1.0"),
+ COMPACTION_SERVICE_ROOT_PLANNER(COMPACTION_SERVICE_PREFIX + "root.planner",
Review Comment:
Are you adding these as a temporary step to get things working? Asking
because of #3981
##########
core/src/main/java/org/apache/accumulo/core/spi/compaction/DefaultCompactionPlanner.java:
##########
@@ -57,85 +57,47 @@
* compaction service you are configuring.
*
* <ul>
- * <li>{@code compaction.service.<service>.opts.executors} This is a json
array of objects where
- * each object has the fields:
+ * <li>Note that the CompactionCoordinator and at least one Compactor for
"Large" must be running.
+ * <li>{@code compaction.service.<service>.opts.maxOpen} This determines the
maximum number of files
+ * that will be included in a single compaction.
+ * <li>{@code compaction.service.<service>.opts.groups} This is a json array
of group objects which
+ * have the following fields:
* <table>
- * <caption>Default Compaction Planner Executor options</caption>
+ * <caption>Default Compaction Planner Queue options</caption>
* <tr>
* <th>Field Name</th>
* <th>Description</th>
* </tr>
* <tr>
* <td>name</td>
- * <td>name or alias of the executor (required)</td>
- * </tr>
- * <tr>
- * <td>type</td>
- * <td>valid values 'internal' or 'external' (required)</td>
+ * <td>name or alias of the group (required)</td>
Review Comment:
Why mention alias here? The feels like there is some support for group name
indirection elsewhere.
##########
core/src/main/java/org/apache/accumulo/core/spi/compaction/DefaultCompactionPlanner.java:
##########
@@ -57,85 +57,47 @@
* compaction service you are configuring.
*
* <ul>
- * <li>{@code compaction.service.<service>.opts.executors} This is a json
array of objects where
- * each object has the fields:
+ * <li>Note that the CompactionCoordinator and at least one Compactor for
"Large" must be running.
Review Comment:
What is `"Large"`?
##########
core/src/test/java/org/apache/accumulo/core/spi/compaction/DefaultCompactionPlannerTest.java:
##########
@@ -586,73 +501,14 @@ public void testErrorOnlyOneMaxSize() {
EasyMock.expect(senv.getConfiguration()).andReturn(conf).anyTimes();
EasyMock.replay(conf, senv);
- String executors = getExecutors("'type':
'internal','maxSize':'32M','numThreads':1",
- "'type': 'internal','numThreads':2", "'type':
'external','group':'q1'");
+ String groups =
+ "[{\"name\":\"small\", \"maxSize\":\"32M\"}, {\"name\":\"medium\"},
{\"name\":\"large\"}]";
var e = assertThrows(IllegalArgumentException.class,
- () -> planner.init(getInitParams(senv, executors)), "Failed to throw
error");
+ () -> planner.init(getInitParams(senv, groups)), "Failed to throw
error");
assertTrue(e.getMessage().contains("maxSize"), "Error message didn't
contain maxSize");
}
- /**
- * Tests executors can only have one without a max size.
- */
- @Test
- public void testErrorDuplicateMaxSize() {
Review Comment:
Maybe the test moved and I did not see it. Seems like it would still be
useful to have a test for groups with duplicate max sizes.
##########
core/src/main/java/org/apache/accumulo/core/util/compaction/CompactionServicesConfig.java:
##########
@@ -97,54 +70,21 @@ public CompactionServicesConfig(AccumuloConfiguration
aconf) {
this(getConfiguration(aconf::getAllPropertiesWithPrefixStripped),
aconf::isPropertySet);
}
- @SuppressWarnings("removal")
private CompactionServicesConfig(Map<String,Map<String,String>> configs,
Predicate<Property> isSetPredicate) {
configs.forEach((prefix, props) -> {
props.forEach((prop, val) -> {
String[] tokens = prop.split("\\.");
if (tokens.length == 2 && tokens[1].equals("planner")) {
- if (prefix.equals(oldPrefix.getKey())) {
- // Log a warning if the old prefix planner is defined by a user.
- Property userDefined = null;
- try {
- userDefined = Property.valueOf(prefix + prop);
- } catch (IllegalArgumentException e) {
- log.trace("Property: {} is not set by default configuration",
prefix + prop);
- }
- boolean isPropSet = true;
- if (userDefined != null) {
- isPropSet = isSetPredicate.test(userDefined);
- }
- if (isPropSet) {
- log.warn(
- "Found compaction planner '{}' using a deprecated prefix.
Please update property to use the '{}' prefix",
- tokens[0], newPrefix);
- }
- }
plannerPrefixes.put(tokens[0], prefix);
planners.put(tokens[0], val);
- }
- });
- });
-
- // Now find all compaction planner options.
- configs.forEach((prefix, props) -> {
- props.forEach((prop, val) -> {
- String[] tokens = prop.split("\\.");
- if (!plannerPrefixes.containsKey(tokens[0])) {
- throw new IllegalArgumentException(
- "Incomplete compaction service definition, missing planner
class: " + prop);
- }
- if (tokens.length == 4 && tokens[1].equals("planner") &&
tokens[2].equals("opts")) {
+ } else if (tokens.length == 4 && tokens[1].equals("planner") &&
tokens[2].equals("opts")) {
Review Comment:
Getting a bit lost in these diff. Will this code still fail if a service is
configured with planner options and no planner class?
--
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: notifications-unsubscr...@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
