keith-turner commented on code in PR #4480:
URL: https://github.com/apache/accumulo/pull/4480#discussion_r1599194523
##########
server/base/src/main/java/org/apache/accumulo/server/compaction/CompactionJobGenerator.java:
##########
@@ -205,9 +209,22 @@ private Collection<CompactionJob>
planCompactions(CompactionServiceId serviceId,
// remove any files that are in active compactions
tablet.getExternalCompactions().values().stream().flatMap(ecm ->
ecm.getJobFiles().stream())
.forEach(tmpFiles::remove);
- // remove any files that are selected
- if (tablet.getSelectedFiles() != null) {
- tmpFiles.keySet().removeAll(tablet.getSelectedFiles().getFiles());
+ // remove any files that are selected and the user compaction has
completed
+ // at least 1 job, otherwise we can keep the files
+ var selectedFiles = tablet.getSelectedFiles();
+
+ long selectedExpirationDuration =
+
ConfigurationTypeHelper.getTimeInMillis(env.getConfiguration(tablet.getTableId())
+ .get(Property.TABLE_COMPACTION_SELECTION_EXPIRATION.getKey()));
+
+ // If jobs are completed, or selected time has not expired, the remove
+ // from the candidate list otherwise we can cancel the selection
+ if (selectedFiles != null) {
+ if (selectedFiles.getCompletedJobs() > 0 ||
(selectedFiles.getSelectedTime() != null
+ && (System.nanoTime() -
selectedFiles.getSelectedTime().getNanos())
Review Comment:
Seems like nanoTime and steady time are being compared. Also
selectedExpirationDuration is in millils, so need to use millils when comparing.
```suggestion
&& steadyTime.minus(selectedFiles.getSelectedTime()).toMillis()
```
##########
server/base/src/main/java/org/apache/accumulo/server/compaction/CompactionJobGenerator.java:
##########
@@ -205,9 +209,22 @@ private Collection<CompactionJob>
planCompactions(CompactionServiceId serviceId,
// remove any files that are in active compactions
tablet.getExternalCompactions().values().stream().flatMap(ecm ->
ecm.getJobFiles().stream())
.forEach(tmpFiles::remove);
- // remove any files that are selected
- if (tablet.getSelectedFiles() != null) {
- tmpFiles.keySet().removeAll(tablet.getSelectedFiles().getFiles());
+ // remove any files that are selected and the user compaction has
completed
+ // at least 1 job, otherwise we can keep the files
+ var selectedFiles = tablet.getSelectedFiles();
+
+ long selectedExpirationDuration =
Review Comment:
Could move this into the `if (selectedFiles != null) {` to avoid looking up
this config when its not needed.
##########
server/manager/src/main/java/org/apache/accumulo/manager/compaction/coordinator/CompactionCoordinator.java:
##########
@@ -458,7 +458,8 @@ public static boolean canReserveCompaction(TabletMetadata
tablet, CompactionKind
tablet.getExtent(), userRequestedCompactions);
return false;
} else if (tablet.getSelectedFiles() != null
- && !Collections.disjoint(jobFiles,
tablet.getSelectedFiles().getFiles())) {
+ && (tablet.getSelectedFiles().getCompletedJobs() > 0
Review Comment:
Could simplify this else if to just
```java
} else if (!Collections.disjoint(jobFiles,
getFilesResevedBySelection(tablet, steadyTime, ctx))) {
```
with this function
```java
private static Set<StoredTabletFile>
getFilesResevedBySelection(TabletMetadata tabletMetadata, SteadyTime
steadyTime, ServerContext ctx) {
if(tabletMetadata.getSelectedFiles() == null) {
return Set.of();
}
if(tabletMetadata.getSelectedFiles().getCompletedJobs() > 0) {
return tabletMetadata.getSelectedFiles().getFiles();
}
long selectedExpirationDuration =
ctx.getTableConfiguration(tabletMetadata.getTableId()).getTimeInMillis(Property.TABLE_COMPACTION_SELECTION_EXPIRATION);
if(steadyTime.minus(tabletMetadata.getSelectedFiles().getSelectedTime()).toMillis()
> selectedExpirationDuration) {
return tabletMetadata.getSelectedFiles().getFiles();
}
return Set.of();
}
```
this uses the behavior of the empty set w/ Collections.disjoint to make the
"else if" simpler. It also includes checks for expiration.
##########
server/manager/src/main/java/org/apache/accumulo/manager/compaction/coordinator/CompactionCoordinator.java:
##########
@@ -561,6 +562,15 @@ protected CompactionMetadata
reserveCompaction(CompactionJobQueues.MetaJob metaJ
var tabletMutator =
tabletsMutator.mutateTablet(extent).requireAbsentOperation()
.requireSame(tabletMetadata, FILES, SELECTED, ECOMP);
+ if (metaJob.getJob().getKind() == CompactionKind.SYSTEM) {
+ var selectedFiles = tabletMetadata.getSelectedFiles();
+ if (selectedFiles != null && (selectedFiles.getCompletedJobs() == 0
+ && !Collections.disjoint(jobFiles, selectedFiles.getFiles()))) {
Review Comment:
May be able to use the ` getFilesResevedBySelection` function mentioned in
another comment to simplify this a bit and consider expiration.
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/SelectedFiles.java:
##########
@@ -44,27 +46,47 @@ public class SelectedFiles {
private final Set<StoredTabletFile> files;
private final boolean initiallySelectedAll;
private final FateId fateId;
+ private final int completedJobs;
+ private final SteadyTime selectedTime;
private String metadataValue;
private static final Gson GSON = new GsonBuilder()
.registerTypeAdapter(SelectedFiles.class, new
SelectedFilesTypeAdapter()).create();
- public SelectedFiles(Set<StoredTabletFile> files, boolean
initiallySelectedAll, FateId fateId) {
+ public SelectedFiles(Set<StoredTabletFile> files, boolean
initiallySelectedAll, FateId fateId,
+ SteadyTime selectedTime) {
+ this(files, initiallySelectedAll, fateId, 0, selectedTime);
+ }
+
+ public SelectedFiles(Set<StoredTabletFile> files, boolean
initiallySelectedAll, FateId fateId,
+ int completedJobs, SteadyTime selectedTime) {
Preconditions.checkArgument(files != null && !files.isEmpty());
this.files = Set.copyOf(files);
this.initiallySelectedAll = initiallySelectedAll;
this.fateId = fateId;
+ this.completedJobs = completedJobs;
+ this.selectedTime = selectedTime;
}
private static class SelectedFilesTypeAdapter extends
TypeAdapter<SelectedFiles> {
+ // These fields could be moved to an enum but for now just using static
Strings
+ // seems better to avoid having to construct an enum each time the string
is read
+ private static final String FATE_ID = "fateId";
+ private static final String SELECTED_ALL = "selAll";
+ private static final String COMPLETED_JOBS = "compJobs";
+ private static final String FILES = "files";
+ private static final String SELECTED_TIME = "selTime";
+
@Override
public void write(JsonWriter out, SelectedFiles selectedFiles) throws
IOException {
out.beginObject();
- out.name("fateId").value(selectedFiles.getFateId().canonical());
- out.name("selAll").value(selectedFiles.initiallySelectedAll());
- out.name("files").beginArray();
+ out.name(FATE_ID).value(selectedFiles.getFateId().canonical());
+ out.name(SELECTED_ALL).value(selectedFiles.initiallySelectedAll());
+ out.name(COMPLETED_JOBS).value(selectedFiles.getCompletedJobs());
+
out.name(SELECTED_TIME).value(selectedFiles.getSelectedTime().getNanos());
Review Comment:
When serializing could use Duration functionality, then the serialized data
would be a number and unit instead of just a number w/o a unit. Was playing
around w/ this in jshell. One downside of this is its probably more expensive
to serialized/deserialize.
```
jshell> import java.time.Duration
jshell> var d = Duration.ofNanos(42);
d ==> PT0.000000042S
jshell> var d2 = Duration.parse("PT0.000000042S")
d2 ==> PT0.000000042S
jshell> d.equals(d2)
$4 ==> true
```
--
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]
