keith-turner commented on code in PR #5026:
URL: https://github.com/apache/accumulo/pull/5026#discussion_r1828504244
##########
core/src/main/java/org/apache/accumulo/core/util/compaction/CompactionJobPrioritizer.java:
##########
@@ -19,42 +19,120 @@
package org.apache.accumulo.core.util.compaction;
import java.util.Comparator;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+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 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) {
+ private static final Map<Pair<TableId,CompactionKind>,Pair<Short,Short>>
SYSTEM_TABLE_RANGES =
+ new HashMap<>();
+ private static final Map<Pair<NamespaceId,CompactionKind>,
+ Pair<Short,Short>> ACCUMULO_NAMESPACE_RANGES = new HashMap<>();
+
+ // Create ranges of possible priority values where each range has
+ // 2000 possible values. Give higher priority to tables where
+ // they have more files than allowed, user compactions over system
+ // compactions, root table over metadata table, metadata table over
+ // other system tables and user tables.
+ private static final Short TABLE_OVER_SIZE_MAX = Short.MAX_VALUE;
+ private static final Short ROOT_TABLE_USER_MAX = Short.MAX_VALUE - 2001;
+ private static final Short ROOT_TABLE_SYSTEM_MAX = (short)
(ROOT_TABLE_USER_MAX - 2001);
+ private static final Short META_TABLE_USER_MAX = (short)
(ROOT_TABLE_SYSTEM_MAX - 2001);
+ private static final Short META_TABLE_SYSTEM_MAX = (short)
(META_TABLE_USER_MAX - 2001);
+ private static final Short SYSTEM_NS_USER_MAX = (short)
(META_TABLE_SYSTEM_MAX - 2001);
+ private static final Short SYSTEM_NS_SYSTEM_MAX = (short)
(SYSTEM_NS_USER_MAX - 2001);
+ private static final Short USER_TABLE_USER_MAX = (short)
(SYSTEM_NS_SYSTEM_MAX - 2001);
+ private static final Short USER_TABLE_SYSTEM_MAX = 0;
+
+ static final Pair<Short,Short> TABLE_OVER_SIZE =
+ new Pair<>((short) (ROOT_TABLE_USER_MAX + 1), TABLE_OVER_SIZE_MAX);
+
+ static final Pair<Short,Short> ROOT_TABLE_USER =
+ new Pair<>((short) (ROOT_TABLE_SYSTEM_MAX + 1), ROOT_TABLE_USER_MAX);
+ static final Pair<Short,Short> ROOT_TABLE_SYSTEM =
+ new Pair<>((short) (META_TABLE_USER_MAX + 1), ROOT_TABLE_SYSTEM_MAX);
+
+ static final Pair<Short,Short> METADATA_TABLE_USER =
+ new Pair<>((short) (META_TABLE_SYSTEM_MAX + 1), META_TABLE_USER_MAX);
+ static final Pair<Short,Short> METADATA_TABLE_SYSTEM =
+ new Pair<>((short) (SYSTEM_NS_USER_MAX + 1), META_TABLE_SYSTEM_MAX);
+
+ static final Pair<Short,Short> SYSTEM_NS_USER =
+ new Pair<>((short) (SYSTEM_NS_SYSTEM_MAX + 1), SYSTEM_NS_USER_MAX);
+ static final Pair<Short,Short> SYSTEM_NS_SYSTEM =
+ new Pair<>((short) (USER_TABLE_USER_MAX + 1), SYSTEM_NS_SYSTEM_MAX);
+
+ static final Pair<Short,Short> USER_TABLE_USER =
+ new Pair<>((short) (USER_TABLE_SYSTEM_MAX + 1), USER_TABLE_USER_MAX);
+ static final Pair<Short,Short> USER_TABLE_SYSTEM =
+ new Pair<>(Short.MIN_VALUE, USER_TABLE_SYSTEM_MAX);
- int prio = totalFiles + compactingFiles;
+ 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);
- 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;
+ // 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(NamespaceId nsId, TableId tableId,
CompactionKind kind,
+ int totalFiles, int compactingFiles, 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);
+
+ if (totalFiles > maxFilesPerTablet && kind == CompactionKind.SYSTEM) {
+ int priority =
+ TABLE_OVER_SIZE.getFirst() + compactingFiles + (totalFiles -
maxFilesPerTablet);
+ if (priority > Short.MAX_VALUE) {
+ return Short.MAX_VALUE;
+ }
Review Comment:
```suggestion
if (priority > TABLE_OVER_SIZE.getSecond()) {
return TABLE_OVER_SIZE.getSecond();
}
```
##########
core/src/main/java/org/apache/accumulo/core/util/compaction/CompactionJobPrioritizer.java:
##########
@@ -19,42 +19,120 @@
package org.apache.accumulo.core.util.compaction;
import java.util.Comparator;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+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 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) {
+ private static final Map<Pair<TableId,CompactionKind>,Pair<Short,Short>>
SYSTEM_TABLE_RANGES =
+ new HashMap<>();
+ private static final Map<Pair<NamespaceId,CompactionKind>,
+ Pair<Short,Short>> ACCUMULO_NAMESPACE_RANGES = new HashMap<>();
+
+ // Create ranges of possible priority values where each range has
+ // 2000 possible values. Give higher priority to tables where
+ // they have more files than allowed, user compactions over system
+ // compactions, root table over metadata table, metadata table over
+ // other system tables and user tables.
+ private static final Short TABLE_OVER_SIZE_MAX = Short.MAX_VALUE;
Review Comment:
Not sure these compactions should be given higher priority than root and
metadata compactions. Maybe this range could be moved between
SYSTEM_NS_SYSTEM_MAX and USER_TABLE_USER_MAX. Then the TABLE_OVER_SIZE range
would only be used non accumulo namespaces.
##########
core/src/main/java/org/apache/accumulo/core/util/compaction/CompactionJobPrioritizer.java:
##########
@@ -19,42 +19,120 @@
package org.apache.accumulo.core.util.compaction;
import java.util.Comparator;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+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 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) {
+ private static final Map<Pair<TableId,CompactionKind>,Pair<Short,Short>>
SYSTEM_TABLE_RANGES =
+ new HashMap<>();
+ private static final Map<Pair<NamespaceId,CompactionKind>,
+ Pair<Short,Short>> ACCUMULO_NAMESPACE_RANGES = new HashMap<>();
+
+ // Create ranges of possible priority values where each range has
+ // 2000 possible values. Give higher priority to tables where
+ // they have more files than allowed, user compactions over system
+ // compactions, root table over metadata table, metadata table over
+ // other system tables and user tables.
+ private static final Short TABLE_OVER_SIZE_MAX = Short.MAX_VALUE;
+ private static final Short ROOT_TABLE_USER_MAX = Short.MAX_VALUE - 2001;
+ private static final Short ROOT_TABLE_SYSTEM_MAX = (short)
(ROOT_TABLE_USER_MAX - 2001);
+ private static final Short META_TABLE_USER_MAX = (short)
(ROOT_TABLE_SYSTEM_MAX - 2001);
+ private static final Short META_TABLE_SYSTEM_MAX = (short)
(META_TABLE_USER_MAX - 2001);
+ private static final Short SYSTEM_NS_USER_MAX = (short)
(META_TABLE_SYSTEM_MAX - 2001);
+ private static final Short SYSTEM_NS_SYSTEM_MAX = (short)
(SYSTEM_NS_USER_MAX - 2001);
+ private static final Short USER_TABLE_USER_MAX = (short)
(SYSTEM_NS_SYSTEM_MAX - 2001);
+ private static final Short USER_TABLE_SYSTEM_MAX = 0;
+
+ static final Pair<Short,Short> TABLE_OVER_SIZE =
+ new Pair<>((short) (ROOT_TABLE_USER_MAX + 1), TABLE_OVER_SIZE_MAX);
+
+ static final Pair<Short,Short> ROOT_TABLE_USER =
+ new Pair<>((short) (ROOT_TABLE_SYSTEM_MAX + 1), ROOT_TABLE_USER_MAX);
+ static final Pair<Short,Short> ROOT_TABLE_SYSTEM =
+ new Pair<>((short) (META_TABLE_USER_MAX + 1), ROOT_TABLE_SYSTEM_MAX);
+
+ static final Pair<Short,Short> METADATA_TABLE_USER =
+ new Pair<>((short) (META_TABLE_SYSTEM_MAX + 1), META_TABLE_USER_MAX);
+ static final Pair<Short,Short> METADATA_TABLE_SYSTEM =
+ new Pair<>((short) (SYSTEM_NS_USER_MAX + 1), META_TABLE_SYSTEM_MAX);
+
+ static final Pair<Short,Short> SYSTEM_NS_USER =
+ new Pair<>((short) (SYSTEM_NS_SYSTEM_MAX + 1), SYSTEM_NS_USER_MAX);
+ static final Pair<Short,Short> SYSTEM_NS_SYSTEM =
+ new Pair<>((short) (USER_TABLE_USER_MAX + 1), SYSTEM_NS_SYSTEM_MAX);
+
+ static final Pair<Short,Short> USER_TABLE_USER =
+ new Pair<>((short) (USER_TABLE_SYSTEM_MAX + 1), USER_TABLE_USER_MAX);
+ static final Pair<Short,Short> USER_TABLE_SYSTEM =
+ new Pair<>(Short.MIN_VALUE, USER_TABLE_SYSTEM_MAX);
- int prio = totalFiles + compactingFiles;
+ 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);
- 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;
+ // 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(NamespaceId nsId, TableId tableId,
CompactionKind kind,
+ int totalFiles, int compactingFiles, 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);
+
+ if (totalFiles > maxFilesPerTablet && kind == CompactionKind.SYSTEM) {
+ int priority =
+ TABLE_OVER_SIZE.getFirst() + compactingFiles + (totalFiles -
maxFilesPerTablet);
+ if (priority > Short.MAX_VALUE) {
+ return Short.MAX_VALUE;
+ }
+ return (short) priority;
+ } else {
+ Pair<Short,Short> range = null;
+ if (Namespace.ACCUMULO.id() == nsId) {
+ range = SYSTEM_TABLE_RANGES.get(new Pair<>(tableId, kind));
+ if (range == null) {
+ range = ACCUMULO_NAMESPACE_RANGES.get(new Pair<>(nsId, kind));
}
- 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 {
+ if (kind == CompactionKind.SYSTEM) {
Review Comment:
The following would line up with what the old code was doing. May be able
to revert some changes to one of the unit test with this change.
```suggestion
if (kind == CompactionKind.SYSTEM || kind ==
CompactionKind.SELECTOR) {
```
##########
core/src/main/java/org/apache/accumulo/core/spi/compaction/CompactionPlanner.java:
##########
@@ -79,6 +81,8 @@ public interface InitParameters {
*/
public interface PlanningParameters {
+ NamespaceId getNamespaceId() throws TableNotFoundException;
Review Comment:
This is SPI code so should add a since tag.
```suggestion
/**
* @since 2.1.4
*/
NamespaceId getNamespaceId() throws TableNotFoundException;
```
##########
core/src/main/java/org/apache/accumulo/core/util/compaction/CompactionJobPrioritizer.java:
##########
@@ -19,42 +19,120 @@
package org.apache.accumulo.core.util.compaction;
import java.util.Comparator;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+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 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) {
+ private static final Map<Pair<TableId,CompactionKind>,Pair<Short,Short>>
SYSTEM_TABLE_RANGES =
+ new HashMap<>();
+ private static final Map<Pair<NamespaceId,CompactionKind>,
+ Pair<Short,Short>> ACCUMULO_NAMESPACE_RANGES = new HashMap<>();
+
+ // Create ranges of possible priority values where each range has
+ // 2000 possible values. Give higher priority to tables where
+ // they have more files than allowed, user compactions over system
+ // compactions, root table over metadata table, metadata table over
+ // other system tables and user tables.
+ private static final Short TABLE_OVER_SIZE_MAX = Short.MAX_VALUE;
+ private static final Short ROOT_TABLE_USER_MAX = Short.MAX_VALUE - 2001;
+ private static final Short ROOT_TABLE_SYSTEM_MAX = (short)
(ROOT_TABLE_USER_MAX - 2001);
+ private static final Short META_TABLE_USER_MAX = (short)
(ROOT_TABLE_SYSTEM_MAX - 2001);
+ private static final Short META_TABLE_SYSTEM_MAX = (short)
(META_TABLE_USER_MAX - 2001);
+ private static final Short SYSTEM_NS_USER_MAX = (short)
(META_TABLE_SYSTEM_MAX - 2001);
+ private static final Short SYSTEM_NS_SYSTEM_MAX = (short)
(SYSTEM_NS_USER_MAX - 2001);
+ private static final Short USER_TABLE_USER_MAX = (short)
(SYSTEM_NS_SYSTEM_MAX - 2001);
+ private static final Short USER_TABLE_SYSTEM_MAX = 0;
+
+ static final Pair<Short,Short> TABLE_OVER_SIZE =
Review Comment:
Not sure if this would be an improvement. Was wondering if creating a
little inner class like the following and using that would clean up the code a
bit over use Pair.
```java
static class PrioRange {
final short min;
final short max;
PrioRange(short min, short max){this.min=min;this.max=max;}
}
```
--
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]
