You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "dlmarion (via GitHub)" <gi...@apache.org> on 2026/03/16 18:44:37 UTC
[PR] Use TExternalCompaction instead of RunningCompaction [accumulo]
dlmarion opened a new pull request, #6221:
URL: https://github.com/apache/accumulo/pull/6221
This change removes the RunningCompaction object and replaces it with TExternalCompaction. Both objects were similar except for the startTime, which I added to TExternalCompaction.
This change will make removing the collections from the coordinator easier in subsequent changes while keeping the same amount of information.
--
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] Use TExternalCompaction instead of RunningCompaction [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6221:
URL: https://github.com/apache/accumulo/pull/6221#discussion_r2955372462
##########
server/manager/src/main/java/org/apache/accumulo/manager/compaction/coordinator/CompactionCoordinator.java:
##########
@@ -407,21 +406,20 @@ public void run() {
// the external compaction came from to re-populate the RUNNING collection.
LOG.info("Checking for running external compactions");
// On re-start contact the running Compactors to try and seed the list of running compactions
- List<RunningCompaction> running = getCompactionsRunningOnCompactors();
+ List<TExternalCompaction> running = getCompactionsRunningOnCompactors();
if (running.isEmpty()) {
LOG.info("No running external compactions found");
} else {
LOG.info("Found {} running external compactions", running.size());
- running.forEach(rc -> {
+ running.forEach(tec -> {
TCompactionStatusUpdate update = new TCompactionStatusUpdate();
update.setState(TCompactionState.IN_PROGRESS);
update.setMessage(RESTART_UPDATE_MSG);
- rc.addUpdate(System.currentTimeMillis(), update);
- rc.setStartTime(this.coordinatorStartTime);
- RUNNING_CACHE.put(ExternalCompactionId.of(rc.getJob().getExternalCompactionId()), rc);
+ tec.putToUpdates(coordinatorStartTime, update);
+ RUNNING_CACHE.put(ExternalCompactionId.of(tec.getJob().getExternalCompactionId()), tec);
LONG_RUNNING_COMPACTIONS_BY_RG
- .computeIfAbsent(rc.getGroup().canonical(), k -> new TimeOrderedRunningCompactionSet())
- .add(rc);
+ .computeIfAbsent(tec.getGroupName(), k -> new TimeOrderedRunningCompactionSet())
+ .add(tec);
Review Comment:
I think you are right that startTime is not set on the TExternalCompaction objects in the RUNNING_CACHE. Looking at the code, I don't think it's using the startTime in the coordinator code. In #6223, I'm moving the long running map to the Monitor, I think that might be the only thing using the startTime, but in #6223 it's getting the TExternalCompaction directly from the compactors instead of the coordinator. I'll see if I can clean this up a bit in #6223.
--
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] Use TExternalCompaction instead of RunningCompaction [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion merged PR #6221:
URL: https://github.com/apache/accumulo/pull/6221
--
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] Use TExternalCompaction instead of RunningCompaction [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6221:
URL: https://github.com/apache/accumulo/pull/6221#discussion_r2942825143
##########
server/compactor/src/main/java/org/apache/accumulo/compactor/Compactor.java:
##########
@@ -313,48 +314,52 @@ protected void startCancelChecker(ScheduledThreadPoolExecutor schedExecutor,
}
protected void checkIfCanceled() {
- TExternalCompactionJob job = JOB_HOLDER.getJob();
- if (job != null) {
- try {
- var extent = KeyExtent.fromThrift(job.getExtent());
- var ecid = ExternalCompactionId.of(job.getExternalCompactionId());
-
- TabletMetadata tabletMeta =
- getContext().getAmple().readTablet(extent, ColumnType.ECOMP, ColumnType.PREV_ROW);
- if (tabletMeta == null || !tabletMeta.getExternalCompactions().containsKey(ecid)) {
- // table was deleted OR tablet was split or merged OR tablet no longer thinks compaction
- // is running for some reason
- LOG.info("Cancelling compaction {} that no longer has a metadata entry at {}", ecid,
- extent);
- JOB_HOLDER.cancel(job.getExternalCompactionId());
- return;
- }
+ if (JOB_HOLDER.getCurrentCompaction() == null) {
+ return;
+ }
+ TExternalCompactionJob job = JOB_HOLDER.getCurrentCompaction().getJob();
Review Comment:
Updated in 9efbe92
--
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] Use TExternalCompaction instead of RunningCompaction [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6221:
URL: https://github.com/apache/accumulo/pull/6221#discussion_r2942700597
##########
server/compactor/src/main/java/org/apache/accumulo/compactor/Compactor.java:
##########
@@ -313,48 +314,52 @@ protected void startCancelChecker(ScheduledThreadPoolExecutor schedExecutor,
}
protected void checkIfCanceled() {
- TExternalCompactionJob job = JOB_HOLDER.getJob();
- if (job != null) {
- try {
- var extent = KeyExtent.fromThrift(job.getExtent());
- var ecid = ExternalCompactionId.of(job.getExternalCompactionId());
-
- TabletMetadata tabletMeta =
- getContext().getAmple().readTablet(extent, ColumnType.ECOMP, ColumnType.PREV_ROW);
- if (tabletMeta == null || !tabletMeta.getExternalCompactions().containsKey(ecid)) {
- // table was deleted OR tablet was split or merged OR tablet no longer thinks compaction
- // is running for some reason
- LOG.info("Cancelling compaction {} that no longer has a metadata entry at {}", ecid,
- extent);
- JOB_HOLDER.cancel(job.getExternalCompactionId());
- return;
- }
+ if (JOB_HOLDER.getCurrentCompaction() == null) {
+ return;
+ }
+ TExternalCompactionJob job = JOB_HOLDER.getCurrentCompaction().getJob();
Review Comment:
This is just being defensive. I can update it so that `getCurrentCompaction()` is only called once.
--
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] Use TExternalCompaction instead of RunningCompaction [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #6221:
URL: https://github.com/apache/accumulo/pull/6221#discussion_r2942518527
##########
server/compactor/src/main/java/org/apache/accumulo/compactor/Compactor.java:
##########
@@ -313,48 +314,52 @@ protected void startCancelChecker(ScheduledThreadPoolExecutor schedExecutor,
}
protected void checkIfCanceled() {
- TExternalCompactionJob job = JOB_HOLDER.getJob();
- if (job != null) {
- try {
- var extent = KeyExtent.fromThrift(job.getExtent());
- var ecid = ExternalCompactionId.of(job.getExternalCompactionId());
-
- TabletMetadata tabletMeta =
- getContext().getAmple().readTablet(extent, ColumnType.ECOMP, ColumnType.PREV_ROW);
- if (tabletMeta == null || !tabletMeta.getExternalCompactions().containsKey(ecid)) {
- // table was deleted OR tablet was split or merged OR tablet no longer thinks compaction
- // is running for some reason
- LOG.info("Cancelling compaction {} that no longer has a metadata entry at {}", ecid,
- extent);
- JOB_HOLDER.cancel(job.getExternalCompactionId());
- return;
- }
+ if (JOB_HOLDER.getCurrentCompaction() == null) {
+ return;
+ }
+ TExternalCompactionJob job = JOB_HOLDER.getCurrentCompaction().getJob();
Review Comment:
This is calling `JOB_HOLDER.getCurrentCompaction()` twice and it could change to null after the first call. Would be better to only call it once.
Also is it expected that that JOB_HOLDER.getCurrentCompaction() would be non-null and its job would be null? Or this just defensive code? If this is expected there are other places in the code that call getCurrentCompaction().getJob() w/o checking for null. Maybe could add a `JOB_HOLDER.getJob()` method to consolidate this.
--
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] Use TExternalCompaction instead of RunningCompaction [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #6221:
URL: https://github.com/apache/accumulo/pull/6221#discussion_r2955285671
##########
server/manager/src/main/java/org/apache/accumulo/manager/compaction/coordinator/CompactionCoordinator.java:
##########
@@ -407,21 +406,20 @@ public void run() {
// the external compaction came from to re-populate the RUNNING collection.
LOG.info("Checking for running external compactions");
// On re-start contact the running Compactors to try and seed the list of running compactions
- List<RunningCompaction> running = getCompactionsRunningOnCompactors();
+ List<TExternalCompaction> running = getCompactionsRunningOnCompactors();
if (running.isEmpty()) {
LOG.info("No running external compactions found");
} else {
LOG.info("Found {} running external compactions", running.size());
- running.forEach(rc -> {
+ running.forEach(tec -> {
TCompactionStatusUpdate update = new TCompactionStatusUpdate();
update.setState(TCompactionState.IN_PROGRESS);
update.setMessage(RESTART_UPDATE_MSG);
- rc.addUpdate(System.currentTimeMillis(), update);
- rc.setStartTime(this.coordinatorStartTime);
- RUNNING_CACHE.put(ExternalCompactionId.of(rc.getJob().getExternalCompactionId()), rc);
+ tec.putToUpdates(coordinatorStartTime, update);
+ RUNNING_CACHE.put(ExternalCompactionId.of(tec.getJob().getExternalCompactionId()), tec);
LONG_RUNNING_COMPACTIONS_BY_RG
- .computeIfAbsent(rc.getGroup().canonical(), k -> new TimeOrderedRunningCompactionSet())
- .add(rc);
+ .computeIfAbsent(tec.getGroupName(), k -> new TimeOrderedRunningCompactionSet())
+ .add(tec);
Review Comment:
I commented on the wrong part of the code here on github. What I was thinking is in `getCompactionJob()` a `TExtaernalCompaction` object is created with no startTime set and stores it in `RUNNING_CACHE`. Unless im missing something that code you linked is for a different collection of `TExtaernalCompaction` objects (which do set the startTime).
--
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] Use TExternalCompaction instead of RunningCompaction [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6221:
URL: https://github.com/apache/accumulo/pull/6221#discussion_r2959964417
##########
server/manager/src/main/java/org/apache/accumulo/manager/compaction/coordinator/CompactionCoordinator.java:
##########
@@ -407,21 +406,20 @@ public void run() {
// the external compaction came from to re-populate the RUNNING collection.
LOG.info("Checking for running external compactions");
// On re-start contact the running Compactors to try and seed the list of running compactions
- List<RunningCompaction> running = getCompactionsRunningOnCompactors();
+ List<TExternalCompaction> running = getCompactionsRunningOnCompactors();
if (running.isEmpty()) {
LOG.info("No running external compactions found");
} else {
LOG.info("Found {} running external compactions", running.size());
- running.forEach(rc -> {
+ running.forEach(tec -> {
TCompactionStatusUpdate update = new TCompactionStatusUpdate();
update.setState(TCompactionState.IN_PROGRESS);
update.setMessage(RESTART_UPDATE_MSG);
- rc.addUpdate(System.currentTimeMillis(), update);
- rc.setStartTime(this.coordinatorStartTime);
- RUNNING_CACHE.put(ExternalCompactionId.of(rc.getJob().getExternalCompactionId()), rc);
+ tec.putToUpdates(coordinatorStartTime, update);
+ RUNNING_CACHE.put(ExternalCompactionId.of(tec.getJob().getExternalCompactionId()), tec);
LONG_RUNNING_COMPACTIONS_BY_RG
- .computeIfAbsent(rc.getGroup().canonical(), k -> new TimeOrderedRunningCompactionSet())
- .add(rc);
+ .computeIfAbsent(tec.getGroupName(), k -> new TimeOrderedRunningCompactionSet())
+ .add(tec);
Review Comment:
@DomGarguilo - great catch. I went back and reviewed the changes and setting the start time is missing. I created #6225 to add this back. Thanks!
--
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