keith-turner commented on code in PR #5026:
URL: https://github.com/apache/accumulo/pull/5026#discussion_r1834807496
##########
core/src/test/java/org/apache/accumulo/core/util/compaction/CompactionPrioritizerTest.java:
##########
@@ -28,56 +37,133 @@
import java.util.Optional;
import org.apache.accumulo.core.client.admin.compaction.CompactableFile;
+import org.apache.accumulo.core.clientImpl.Namespace;
+import org.apache.accumulo.core.data.TableId;
+import org.apache.accumulo.core.metadata.MetadataTable;
+import org.apache.accumulo.core.metadata.RootTable;
import org.apache.accumulo.core.spi.compaction.CompactionJob;
import org.apache.accumulo.core.spi.compaction.CompactionKind;
import org.junit.jupiter.api.Test;
public class CompactionPrioritizerTest {
+ private static final int TABLET_FILE_MAX = 3001;
+
public CompactionJob createJob(CompactionKind kind, String tablet, int
numFiles, int totalFiles) {
Collection<CompactableFile> files = new ArrayList<>();
for (int i = 0; i < numFiles; i++) {
files.add(CompactableFile
.create(URI.create("hdfs://foonn/accumulo/tables/5/" + tablet + "/"
+ i + ".rf"), 4, 4));
}
- // TODO pass numFiles
return new CompactionJobImpl(
- CompactionJobPrioritizer.createPriority(kind, totalFiles, numFiles),
+ CompactionJobPrioritizer.createPriority(Namespace.DEFAULT.id(),
TableId.of("5"), kind,
+ totalFiles, numFiles, totalFiles * 2),
CompactionExecutorIdImpl.externalId("test"), files, kind,
Optional.of(false));
}
@Test
- public void testPrioritizer() throws Exception {
- assertEquals((short) 0,
CompactionJobPrioritizer.createPriority(CompactionKind.USER, 0, 0));
- assertEquals((short) 10000,
- CompactionJobPrioritizer.createPriority(CompactionKind.USER, 10000,
0));
- assertEquals((short) 32767,
- CompactionJobPrioritizer.createPriority(CompactionKind.USER, 32767,
0));
- assertEquals((short) 32767,
- CompactionJobPrioritizer.createPriority(CompactionKind.USER,
Integer.MAX_VALUE, 0));
-
- assertEquals((short) -32768,
- CompactionJobPrioritizer.createPriority(CompactionKind.SYSTEM, 0, 0));
- assertEquals((short) -22768,
- CompactionJobPrioritizer.createPriority(CompactionKind.SYSTEM, 10000,
0));
- assertEquals((short) -1,
- CompactionJobPrioritizer.createPriority(CompactionKind.SYSTEM, 32767,
0));
- assertEquals((short) -1,
- CompactionJobPrioritizer.createPriority(CompactionKind.SYSTEM,
Integer.MAX_VALUE, 0));
+ public void testRootTablePriorities() {
Review Comment:
Can add chop compaction kind to these accumulo system table test.
##########
core/src/main/java/org/apache/accumulo/core/util/compaction/CompactionJobPrioritizer.java:
##########
@@ -19,42 +19,127 @@
package org.apache.accumulo.core.util.compaction;
import java.util.Comparator;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.function.Function;
+import org.apache.accumulo.core.clientImpl.Namespace;
+import org.apache.accumulo.core.data.NamespaceId;
+import org.apache.accumulo.core.data.TableId;
+import org.apache.accumulo.core.metadata.MetadataTable;
+import org.apache.accumulo.core.metadata.RootTable;
import org.apache.accumulo.core.spi.compaction.CompactionJob;
import org.apache.accumulo.core.spi.compaction.CompactionKind;
+import org.apache.accumulo.core.util.Pair;
+import org.apache.commons.lang3.Range;
+
+import com.google.common.base.Preconditions;
public class CompactionJobPrioritizer {
public static final Comparator<CompactionJob> JOB_COMPARATOR =
Comparator.comparingInt(CompactionJob::getPriority)
.thenComparingInt(job -> job.getFiles().size()).reversed();
- public static short createPriority(CompactionKind kind, int totalFiles, int
compactingFiles) {
-
- int prio = totalFiles + compactingFiles;
-
- switch (kind) {
- case USER:
- case CHOP:
- // user-initiated compactions will have a positive priority
- // based on number of files
- if (prio > Short.MAX_VALUE) {
- return Short.MAX_VALUE;
- }
- return (short) prio;
- case SELECTOR:
- case SYSTEM:
- // system-initiated compactions will have a negative priority
- // starting at -32768 and increasing based on number of files
- // maxing out at -1
- if (prio > Short.MAX_VALUE) {
- return -1;
- } else {
- return (short) (Short.MIN_VALUE + prio);
- }
- default:
- throw new AssertionError("Unknown kind " + kind);
+ private static final Map<Pair<TableId,CompactionKind>,Range<Short>>
SYSTEM_TABLE_RANGES =
+ new HashMap<>();
+ private static final Map<Pair<NamespaceId,CompactionKind>,
+ Range<Short>> ACCUMULO_NAMESPACE_RANGES = new HashMap<>();
+
+ // Create ranges of possible priority values where each range has
+ // 2000 possible values. Priority order is:
+ // root table user initiated
+ // root table system initiated
+ // metadata table user initiated
+ // metadata table system initiated
+ // other tables in accumulo namespace user initiated
+ // other tables in accumulo namespace system initiated
+ // user tables that have more files that configured system initiated
+ // user tables user initiated
+ // user tables system initiated
+ static final Range<Short> ROOT_TABLE_USER = Range.of((short) 30768, (short)
32767);
+ static final Range<Short> ROOT_TABLE_SYSTEM = Range.of((short) 28768,
(short) 30767);
+
+ static final Range<Short> METADATA_TABLE_USER = Range.of((short) 26768,
(short) 28767);
+ static final Range<Short> METADATA_TABLE_SYSTEM = Range.of((short) 24768,
(short) 26767);
+
+ static final Range<Short> SYSTEM_NS_USER = Range.of((short) 22768, (short)
24767);
+ static final Range<Short> SYSTEM_NS_SYSTEM = Range.of((short) 20768, (short)
22767);
+
+ static final Range<Short> TABLE_OVER_SIZE = Range.of((short) 18768, (short)
20767);
+
+ static final Range<Short> USER_TABLE_USER = Range.of((short) 1, (short)
18767);
+ static final Range<Short> USER_TABLE_SYSTEM = Range.of((short) -32768,
(short) 0);
+
+ static {
+ // root table
+ SYSTEM_TABLE_RANGES.put(new Pair<>(RootTable.ID, CompactionKind.USER),
ROOT_TABLE_USER);
+ SYSTEM_TABLE_RANGES.put(new Pair<>(RootTable.ID, CompactionKind.SYSTEM),
ROOT_TABLE_SYSTEM);
+
+ // metadata table
+ SYSTEM_TABLE_RANGES.put(new Pair<>(MetadataTable.ID, CompactionKind.USER),
METADATA_TABLE_USER);
+ SYSTEM_TABLE_RANGES.put(new Pair<>(MetadataTable.ID,
CompactionKind.SYSTEM),
+ METADATA_TABLE_SYSTEM);
+
+ // metadata table
+ ACCUMULO_NAMESPACE_RANGES.put(new Pair<>(Namespace.ACCUMULO.id(),
CompactionKind.USER),
+ SYSTEM_NS_USER);
+ ACCUMULO_NAMESPACE_RANGES.put(new Pair<>(Namespace.ACCUMULO.id(),
CompactionKind.SYSTEM),
+ SYSTEM_NS_SYSTEM);
+ }
+
+ public static short createPriority(final NamespaceId nsId, final TableId
tableId,
+ final CompactionKind kind, final int totalFiles, final int
compactingFiles,
+ final int maxFilesPerTablet) {
+
+ Objects.requireNonNull(nsId, "nsId cannot be null");
+ Objects.requireNonNull(tableId, "tableId cannot be null");
+ Preconditions.checkArgument(totalFiles >= 0, "totalFiles is negative %s",
totalFiles);
+ Preconditions.checkArgument(compactingFiles >= 0, "compactingFiles is
negative %s",
+ compactingFiles);
+
+ final Function<Range<Short>,Short> normalPriorityFunction = new
Function<>() {
+ @Override
+ public Short apply(Range<Short> f) {
+ return (short) Math.min(f.getMaximum(), f.getMinimum() + totalFiles +
compactingFiles);
+ }
+ };
+
+ final Function<Range<Short>,Short> tabletOverSizeFunction = new
Function<>() {
+ @Override
+ public Short apply(Range<Short> f) {
+ return (short) Math.min(f.getMaximum(),
+ f.getMinimum() + compactingFiles + (totalFiles -
maxFilesPerTablet));
+ }
+ };
+
+ Range<Short> range = null;
+ Function<Range<Short>,Short> func = normalPriorityFunction;
+ if (Namespace.ACCUMULO.id() == nsId) {
+ // Handle system tables
Review Comment:
There could be chop compactions when merging the metadata table and these
may end w/ a null range causing an exception. Need to add handling for kind of
chop. Will have to drop this in the 3.1 code.
##########
core/src/test/java/org/apache/accumulo/core/util/compaction/CompactionPrioritizerTest.java:
##########
@@ -28,56 +37,133 @@
import java.util.Optional;
import org.apache.accumulo.core.client.admin.compaction.CompactableFile;
+import org.apache.accumulo.core.clientImpl.Namespace;
+import org.apache.accumulo.core.data.TableId;
+import org.apache.accumulo.core.metadata.MetadataTable;
+import org.apache.accumulo.core.metadata.RootTable;
import org.apache.accumulo.core.spi.compaction.CompactionJob;
import org.apache.accumulo.core.spi.compaction.CompactionKind;
import org.junit.jupiter.api.Test;
public class CompactionPrioritizerTest {
+ private static final int TABLET_FILE_MAX = 3001;
+
public CompactionJob createJob(CompactionKind kind, String tablet, int
numFiles, int totalFiles) {
Collection<CompactableFile> files = new ArrayList<>();
for (int i = 0; i < numFiles; i++) {
files.add(CompactableFile
.create(URI.create("hdfs://foonn/accumulo/tables/5/" + tablet + "/"
+ i + ".rf"), 4, 4));
}
- // TODO pass numFiles
return new CompactionJobImpl(
- CompactionJobPrioritizer.createPriority(kind, totalFiles, numFiles),
+ CompactionJobPrioritizer.createPriority(Namespace.DEFAULT.id(),
TableId.of("5"), kind,
+ totalFiles, numFiles, totalFiles * 2),
CompactionExecutorIdImpl.externalId("test"), files, kind,
Optional.of(false));
}
@Test
- public void testPrioritizer() throws Exception {
- assertEquals((short) 0,
CompactionJobPrioritizer.createPriority(CompactionKind.USER, 0, 0));
- assertEquals((short) 10000,
- CompactionJobPrioritizer.createPriority(CompactionKind.USER, 10000,
0));
- assertEquals((short) 32767,
- CompactionJobPrioritizer.createPriority(CompactionKind.USER, 32767,
0));
- assertEquals((short) 32767,
- CompactionJobPrioritizer.createPriority(CompactionKind.USER,
Integer.MAX_VALUE, 0));
-
- assertEquals((short) -32768,
- CompactionJobPrioritizer.createPriority(CompactionKind.SYSTEM, 0, 0));
- assertEquals((short) -22768,
- CompactionJobPrioritizer.createPriority(CompactionKind.SYSTEM, 10000,
0));
- assertEquals((short) -1,
- CompactionJobPrioritizer.createPriority(CompactionKind.SYSTEM, 32767,
0));
- assertEquals((short) -1,
- CompactionJobPrioritizer.createPriority(CompactionKind.SYSTEM,
Integer.MAX_VALUE, 0));
+ public void testRootTablePriorities() {
+ assertEquals(ROOT_TABLE_USER.getMinimum(),
CompactionJobPrioritizer.createPriority(
+ Namespace.ACCUMULO.id(), RootTable.ID, CompactionKind.USER, 0, 0,
TABLET_FILE_MAX));
+ assertEquals(ROOT_TABLE_USER.getMinimum() + 1000,
CompactionJobPrioritizer.createPriority(
+ Namespace.ACCUMULO.id(), RootTable.ID, CompactionKind.USER, 1000, 0,
TABLET_FILE_MAX));
+ assertEquals(ROOT_TABLE_USER.getMaximum(),
CompactionJobPrioritizer.createPriority(
+ Namespace.ACCUMULO.id(), RootTable.ID, CompactionKind.USER, 3000, 0,
TABLET_FILE_MAX));
+ assertEquals(ROOT_TABLE_SYSTEM.getMinimum(),
CompactionJobPrioritizer.createPriority(
+ Namespace.ACCUMULO.id(), RootTable.ID, CompactionKind.SYSTEM, 0, 0,
TABLET_FILE_MAX));
+ assertEquals(ROOT_TABLE_SYSTEM.getMinimum() + 1000,
CompactionJobPrioritizer.createPriority(
+ Namespace.ACCUMULO.id(), RootTable.ID, CompactionKind.SYSTEM, 1000, 0,
TABLET_FILE_MAX));
+ assertEquals(ROOT_TABLE_SYSTEM.getMaximum(),
CompactionJobPrioritizer.createPriority(
+ Namespace.ACCUMULO.id(), RootTable.ID, CompactionKind.SYSTEM, 3000, 0,
TABLET_FILE_MAX));
+ }
+
+ @Test
+ public void testMetaTablePriorities() {
+ assertEquals(METADATA_TABLE_USER.getMinimum(),
CompactionJobPrioritizer.createPriority(
+ Namespace.ACCUMULO.id(), MetadataTable.ID, CompactionKind.USER, 0, 0,
TABLET_FILE_MAX));
+ assertEquals(METADATA_TABLE_USER.getMinimum() + 1000,
CompactionJobPrioritizer.createPriority(
+ Namespace.ACCUMULO.id(), MetadataTable.ID, CompactionKind.USER, 1000,
0, TABLET_FILE_MAX));
+ assertEquals(METADATA_TABLE_USER.getMaximum(),
CompactionJobPrioritizer.createPriority(
+ Namespace.ACCUMULO.id(), MetadataTable.ID, CompactionKind.USER, 3000,
0, TABLET_FILE_MAX));
+ assertEquals(METADATA_TABLE_SYSTEM.getMinimum(),
CompactionJobPrioritizer.createPriority(
+ Namespace.ACCUMULO.id(), MetadataTable.ID, CompactionKind.SYSTEM, 0,
0, TABLET_FILE_MAX));
+ assertEquals(METADATA_TABLE_SYSTEM.getMinimum() + 1000,
+ CompactionJobPrioritizer.createPriority(Namespace.ACCUMULO.id(),
MetadataTable.ID,
+ CompactionKind.SYSTEM, 1000, 0, TABLET_FILE_MAX));
+ assertEquals(METADATA_TABLE_SYSTEM.getMaximum(),
+ CompactionJobPrioritizer.createPriority(Namespace.ACCUMULO.id(),
MetadataTable.ID,
+ CompactionKind.SYSTEM, 3000, 0, TABLET_FILE_MAX));
+ }
+
+ @Test
+ public void testSystemNamespacePriorities() {
+ TableId tid = TableId.of("someOtherSystemTable");
+ assertEquals(SYSTEM_NS_USER.getMinimum(), CompactionJobPrioritizer
+ .createPriority(Namespace.ACCUMULO.id(), tid, CompactionKind.USER, 0,
0, TABLET_FILE_MAX));
+ assertEquals(SYSTEM_NS_USER.getMinimum() + 1000,
CompactionJobPrioritizer.createPriority(
+ Namespace.ACCUMULO.id(), tid, CompactionKind.USER, 1000, 0,
TABLET_FILE_MAX));
Review Comment:
A lot of test use zero for the num compacting files. May be good to use
something greater than zero to ensure its included in computation.
##########
core/src/main/java/org/apache/accumulo/core/util/compaction/CompactionJobPrioritizer.java:
##########
@@ -19,42 +19,127 @@
package org.apache.accumulo.core.util.compaction;
import java.util.Comparator;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.function.Function;
+import org.apache.accumulo.core.clientImpl.Namespace;
+import org.apache.accumulo.core.data.NamespaceId;
+import org.apache.accumulo.core.data.TableId;
+import org.apache.accumulo.core.metadata.MetadataTable;
+import org.apache.accumulo.core.metadata.RootTable;
import org.apache.accumulo.core.spi.compaction.CompactionJob;
import org.apache.accumulo.core.spi.compaction.CompactionKind;
+import org.apache.accumulo.core.util.Pair;
+import org.apache.commons.lang3.Range;
+
+import com.google.common.base.Preconditions;
public class CompactionJobPrioritizer {
public static final Comparator<CompactionJob> JOB_COMPARATOR =
Comparator.comparingInt(CompactionJob::getPriority)
.thenComparingInt(job -> job.getFiles().size()).reversed();
- public static short createPriority(CompactionKind kind, int totalFiles, int
compactingFiles) {
-
- int prio = totalFiles + compactingFiles;
-
- switch (kind) {
- case USER:
- case CHOP:
- // user-initiated compactions will have a positive priority
- // based on number of files
- if (prio > Short.MAX_VALUE) {
- return Short.MAX_VALUE;
- }
- return (short) prio;
- case SELECTOR:
- case SYSTEM:
- // system-initiated compactions will have a negative priority
- // starting at -32768 and increasing based on number of files
- // maxing out at -1
- if (prio > Short.MAX_VALUE) {
- return -1;
- } else {
- return (short) (Short.MIN_VALUE + prio);
- }
- default:
- throw new AssertionError("Unknown kind " + kind);
+ private static final Map<Pair<TableId,CompactionKind>,Range<Short>>
SYSTEM_TABLE_RANGES =
+ new HashMap<>();
+ private static final Map<Pair<NamespaceId,CompactionKind>,
+ Range<Short>> ACCUMULO_NAMESPACE_RANGES = new HashMap<>();
+
+ // Create ranges of possible priority values where each range has
+ // 2000 possible values. Priority order is:
+ // root table user initiated
+ // root table system initiated
+ // metadata table user initiated
+ // metadata table system initiated
+ // other tables in accumulo namespace user initiated
+ // other tables in accumulo namespace system initiated
+ // user tables that have more files that configured system initiated
+ // user tables user initiated
+ // user tables system initiated
+ static final Range<Short> ROOT_TABLE_USER = Range.of((short) 30768, (short)
32767);
Review Comment:
I would be nice to ensure all of these ranges are non overlapping and cover
everything from short min to short max. This may already be happening
indirectly in the unit test, so may not need an explicit test.
--
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]
