You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "cshannon (via GitHub)" <gi...@apache.org> on 2024/04/20 18:00:15 UTC
[PR] Allow system compactions to run if zero user compaction jobs have run [accumulo]
cshannon opened a new pull request, #4480:
URL: https://github.com/apache/accumulo/pull/4480
This change will allow system compactions to postpone user compactions that have had no jobs run yet. Before this, if a user compaction was in the queue and had selected files that overlapped it would block system compactions from running. Now if there are selected files, but the user compaction is not running and hasn't had any jobs completed, the coordinator will clear the selectedFiles column so that the system compaction can run. The fate operation will reset the column again while trying to make progress.
Marking this as a draft for now as there is an outstanding TODO, there's still an open question about how to handle time/expiration, and also more testing is needed as there is only one IT so far.
The current test works by creating two tables and setting compactions to be slow using the slow iterator and also setting system compactions to be disabled. The test will start compacting one table so the compactor is busy. Next a user compaction is started that will waiting in the queue and then the system compactions are re-enabled so a system compaction will be scheduled. A custom test planner is set so that system compactions take higher precedence. This means that when the compation coordinator goes to start the next compaction job, it will try and grab the system compaction ahead of the user compaction. Before this change this would have been blocked by the selectedFiles column being set by the user compaction, however because the user compaction has had no jobs run it can proceed and clear the column and run the system compaction. Later the fate operation will re-set it again and the user compaction will run second.
This closes #4454
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow system compactions to run if zero user compaction jobs have run [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #4480:
URL: https://github.com/apache/accumulo/pull/4480#discussion_r1605548685
##########
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();
+
+ if (selectedFiles != null) {
+ 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.getCompletedJobs() > 0 || (selectedFiles.getSelectedTime() != null
Review Comment:
Yeah it should not be null, I am wondering if we should add a notNull or precondition check in the constructor for SelectedFiles [here](https://github.com/apache/accumulo/pull/4480/files#diff-a1c7c614b8a2a9952143f2f70fd3ff199c433d8d0c429b834effb70f5c5326afR69)
I'm also wondering if we should validate anything else such as completedJobs >= 0 and that FateId is not null
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow system compactions to run if zero user compaction jobs have run [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #4480:
URL: https://github.com/apache/accumulo/pull/4480#issuecomment-2081521588
I started to work on a follow on to this PR in another branch that is based on the SteadyTime changes from #4494 and is here: https://github.com/cshannon/accumulo/commit/19977034c3f0fceb1a3cf0972195ad4fb459a484
It's not tested and just a rough draft of how we could possibly use SteadyTime. I was unsure about a couple of things, first the commit updates CompactionJobGenerator but I'm not sure if we need to also update CompactionCoordinator to use SteadyTime when dealing with selected files. Second, I wasn't sure if the expiration should be an independent check or tied to if compaction jobs have run (ie only expire if compaction jobs run is greater than 0
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow system compactions to run if zero user compaction jobs have run [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #4480:
URL: https://github.com/apache/accumulo/pull/4480#discussion_r1605430255
##########
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:
This has been implemented in the latest commit, dafdc87afc but I just wanted to note that the logic above is backwards. Time time check should be:
```java
if(steadyTime.minus(tabletMetadata.getSelectedFiles().getSelectedTime()).toMillis() < selectedExpirationDuration) {
return tabletMetadata.getSelectedFiles().getFiles();
}
```
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow system compactions to run if zero user compaction jobs have run [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #4480:
URL: https://github.com/apache/accumulo/pull/4480#issuecomment-2105305779
> I started to work on a follow on to this PR in another branch that is based on the SteadyTime changes from #4494 and is here: [cshannon@1997703](https://github.com/cshannon/accumulo/commit/19977034c3f0fceb1a3cf0972195ad4fb459a484)
>
> It's not tested and just a rough draft of how we could possibly use SteadyTime. I was unsure about a couple of things, first the commit updates CompactionJobGenerator but I'm not sure if we need to also update CompactionCoordinator to use SteadyTime when dealing with selected files. Second, I wasn't sure if the expiration should be an independent check or tied to if compaction jobs have run (ie only expire if compaction jobs run is greater than 0
I merged in these changes after SteadyTime was merged into elasticity, the same comments/questions apply
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow system compactions to run if zero user compaction jobs have run [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4480:
URL: https://github.com/apache/accumulo/pull/4480#discussion_r1605548755
##########
test/src/main/java/org/apache/accumulo/test/functional/CompactionIT.java:
##########
@@ -1011,6 +1040,117 @@ public void testUserCompactionRequested() throws Exception {
ExternalCompactionTestUtils.assertNoCompactionMetadata(getServerContext(), tableName);
}
+ @Test
+ public void testCancelUserCompaction() throws Exception {
Review Comment:
This is a nice test w/ good comments.
##########
server/manager/src/main/java/org/apache/accumulo/manager/compaction/coordinator/CompactionCoordinator.java:
##########
@@ -561,6 +566,20 @@ 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();
+ var reserved = getFilesReservedBySelection(tabletMetadata, manager.getSteadyTime(), ctx);
+
+ // If there is a selectedFiles column, and the reserved set is empty this means that
+ // either no user jobs were completed yet or the selection expiration time has passed
+ // so the column is eligible to be deleted so a system job can run instead
+ if (selectedFiles != null && reserved.isEmpty()
+ && !Collections.disjoint(jobFiles, selectedFiles.getFiles())) {
+ LOG.debug("Deleting user compaction selected files for {}", extent);
Review Comment:
```suggestion
LOG.debug("Deleting user compaction selected files for {} {}", extent, externalCompactionId);
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/compact/CompactionDriver.java:
##########
@@ -241,17 +242,27 @@ public int updateAndCheckTablets(Manager manager, FateId fateId)
noneSelected++;
} else {
var mutator = tabletsMutator.mutateTablet(tablet.getExtent()).requireAbsentOperation()
- .requireSame(tablet, FILES, SELECTED, ECOMP, COMPACTED);
- var selectedFiles =
- new SelectedFiles(filesToCompact, tablet.getFiles().equals(filesToCompact), fateId);
+ .requireSame(tablet, FILES, SELECTED, ECOMP, COMPACTED, USER_COMPACTION_REQUESTED);
+ var selectedFiles = new SelectedFiles(filesToCompact,
+ tablet.getFiles().equals(filesToCompact), fateId, time);
mutator.putSelectedFiles(selectedFiles);
+ // We no longer need to include this marker if files are selected
+ if (tablet.getUserCompactionsRequested().contains(fateId)) {
+ mutator.deleteUserCompactionRequested(fateId);
+ }
+
selectionsSubmitted.put(tablet.getExtent(), filesToCompact);
+ // TODO: Do we need to handle a race condition for the rejection handler check
Review Comment:
Re this comment. Could relax the check in the rejection handler to `tabletMetadata.getSelectedFiles().getFateId().equals(fateId) || tabletMetadata.getCompacted().contains(fateId)` if either of those are true then write when through, but could have changed since to compact some or even completed.
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/compact/CompactionDriver.java:
##########
@@ -186,6 +186,7 @@ public int updateAndCheckTablets(Manager manager, FateId fateId)
var tabletsMutator = ample.conditionallyMutateTablets(resultConsumer)) {
CompactionConfig config = CompactionConfigStorage.getConfig(manager.getContext(), fateId);
+ var time = manager.getSteadyTime();
Review Comment:
May be better to grab the steady time each time its needed. Thinking for a large amount of tablets it could take a while to scan and write out data and this may get stale. Seems like it ok to have this be different for different tablets. Looking at the impl of manager.getSteadyTime(), it seems like it should be quick to call.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow system compactions to run if zero user compaction jobs have run [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4480:
URL: https://github.com/apache/accumulo/pull/4480#discussion_r1605552721
##########
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();
+
+ if (selectedFiles != null) {
+ 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.getCompletedJobs() > 0 || (selectedFiles.getSelectedTime() != null
Review Comment:
Those sound like good validations to add to the SelectedFiles constructor
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow system compactions to run if zero user compaction jobs have run [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #4480:
URL: https://github.com/apache/accumulo/pull/4480#discussion_r1605431596
##########
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:
Added in https://github.com/apache/accumulo/commit/dafdc87afcab2dee407f8d2fc4953f148963feb1
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow system compactions to run if zero user compaction jobs have run [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4480:
URL: https://github.com/apache/accumulo/pull/4480#discussion_r1605497195
##########
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();
+
+ if (selectedFiles != null) {
+ 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.getCompletedJobs() > 0 || (selectedFiles.getSelectedTime() != null
Review Comment:
Why would `selectedFiles.getSelectedTime()` be null?
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow system compactions to run if zero user compaction jobs have run [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon merged PR #4480:
URL: https://github.com/apache/accumulo/pull/4480
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow system compactions to run if zero user compaction jobs have run [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow system compactions to run if zero user compaction jobs have run [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #4480:
URL: https://github.com/apache/accumulo/pull/4480#discussion_r1605145016
##########
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:
To avoid the serialization impact I renamed SELECTED_TIME to SELECTED_TIME_NANOS which uses the string "selTimeNanos" so it is clear what the value is.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow system compactions to run if zero user compaction jobs have run [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #4480:
URL: https://github.com/apache/accumulo/pull/4480#discussion_r1605141899
##########
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:
Thanks for pointing that out, yeah this was a mistake, I noticed this too today as I was re-looking at this when my IDE was giving me a warning about the SteadyTime object that is a private member never being used. The intent was to use it here just like that and it got missed.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Allow system compactions to run if zero user compaction jobs have run [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #4480:
URL: https://github.com/apache/accumulo/pull/4480#discussion_r1605787376
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/compact/CompactionDriver.java:
##########
@@ -241,17 +242,27 @@ public int updateAndCheckTablets(Manager manager, FateId fateId)
noneSelected++;
} else {
var mutator = tabletsMutator.mutateTablet(tablet.getExtent()).requireAbsentOperation()
- .requireSame(tablet, FILES, SELECTED, ECOMP, COMPACTED);
- var selectedFiles =
- new SelectedFiles(filesToCompact, tablet.getFiles().equals(filesToCompact), fateId);
+ .requireSame(tablet, FILES, SELECTED, ECOMP, COMPACTED, USER_COMPACTION_REQUESTED);
+ var selectedFiles = new SelectedFiles(filesToCompact,
+ tablet.getFiles().equals(filesToCompact), fateId, time);
mutator.putSelectedFiles(selectedFiles);
+ // We no longer need to include this marker if files are selected
+ if (tablet.getUserCompactionsRequested().contains(fateId)) {
+ mutator.deleteUserCompactionRequested(fateId);
+ }
+
selectionsSubmitted.put(tablet.getExtent(), filesToCompact);
+ // TODO: Do we need to handle a race condition for the rejection handler check
Review Comment:
Alright so the full rejection handler should just look like the following? Just want to make sure i have it right.
```java
mutator.submit(tabletMetadata -> tabletMetadata.getSelectedFiles() != null
&& tabletMetadata.getSelectedFiles().getFateId().equals(fateId) ||
tabletMetadata.getCompacted().contains(fateId));
```
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org