Use TExternalCompaction instead of RunningCompaction#6221
Use TExternalCompaction instead of RunningCompaction#6221dlmarion merged 2 commits intoapache:mainfrom
Conversation
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.
server/compactor/src/main/java/org/apache/accumulo/compactor/Compactor.java
Outdated
Show resolved
Hide resolved
| RUNNING_CACHE.forEach((ecid, rc) -> { | ||
| TExternalCompaction trc = new TExternalCompaction(); | ||
| trc.setGroupName(rc.getGroup().canonical()); | ||
| trc.setCompactor(rc.getCompactorAddress()); | ||
| trc.setUpdates(rc.getUpdates()); | ||
| trc.setJob(rc.getJob()); | ||
| result.putToCompactions(ecid.canonical(), trc); | ||
| RUNNING_CACHE.forEach((ecid, tec) -> { | ||
| result.putToCompactions(ecid.canonical(), tec); |
There was a problem hiding this comment.
Looks like we used to build a new object now we use the object directly from the cache. Could this cause CME?
There was a problem hiding this comment.
I don't think so as RUNNING_CACHE is a ConcurrentHashMap. Is there something that you see that might be an issue?
| 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); |
There was a problem hiding this comment.
The start time is not set here anymore. Is that intentional?
There was a problem hiding this 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).
There was a problem hiding this 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.
There was a problem hiding this 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!
In apache#6221 we removed the RunningCompaction object in favor of using TExternalCompaction. The RunningCompaction object was setting its start time variable when the STARTED update msg was added. This behavior was lost in apache#6221. This change adds it back by setting the start time on the TExternalCompaction object when the STARTED msg is received.
In #6221 we removed the RunningCompaction object in favor of using TExternalCompaction. The RunningCompaction object was setting its start time variable when the STARTED update msg was added. This behavior was lost in #6221. This change adds it back by setting the start time on the TExternalCompaction object when the STARTED msg is received.
3 participants
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.