ctubbsii commented on code in PR #5026:
URL: https://github.com/apache/accumulo/pull/5026#discussion_r1831511054
##########
core/src/main/java/org/apache/accumulo/core/util/compaction/CompactionJobPrioritizer.java:
##########
@@ -19,42 +19,167 @@
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 com.google.common.base.Preconditions;
public class CompactionJobPrioritizer {
+ public static class PriorityRange {
+ static PriorityRange of(Short min, Short max) {
+ return new PriorityRange(min, max);
+ }
+
+ final Short min;
+ final Short max;
+
+ private PriorityRange(Short min, Short max) {
+ this.min = min;
+ this.max = max;
+ }
+
+ public Short getMin() {
+ return this.min;
+ }
+
+ public Short getMax() {
+ return this.max;
+ }
+ }
+
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>,PriorityRange>
SYSTEM_TABLE_RANGES =
+ new HashMap<>();
+ private static final Map<Pair<NamespaceId,CompactionKind>,
+ PriorityRange> 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
+ private static final Short ROOT_TABLE_USER_MAX = Short.MAX_VALUE;
+ 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 TABLE_OVER_SIZE_MAX = (short)
(SYSTEM_NS_SYSTEM_MAX - 2001);
+ private static final Short USER_TABLE_USER_MAX = (short)
(TABLE_OVER_SIZE_MAX - 2001);
Review Comment:
It looks like each of these ranges are of size 2001, instead of 2000. It
looks like 2000 was used to handle an edge case, but using it for every range
seems incorrect. However, it might be correct, because later, the
`PriorityRange.of` methods all have a `+ 1` in them.
All of it is very difficult to read.
I think it'd be much easier to read if these constants just got removed, and
the PriorityRange objects were constructed with explicit integer literals,
where it was clear what their ranges were. A unit test could trivially verify
that they are adjacent and non-overlapping with a couple of lines of code.
Guava has a built-in Range type that would be perfect for these, so we don't
need to use our own custom range type.
--
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]
