ctubbsii commented on a change in pull request #2318:
URL: https://github.com/apache/accumulo/pull/2318#discussion_r735896793
##########
File path:
core/src/main/java/org/apache/accumulo/core/clientImpl/InstanceOperationsImpl.java
##########
@@ -228,15 +228,17 @@ public boolean testClassLoad(final String className,
final String asTypeName)
futures.add(executorService.submit(() ->
getActiveCompactions(tserver)));
}
- for (HostAndPort compactorAddr : compactors) {
- Callable<List<ActiveCompaction>> task =
- () -> ExternalCompactionUtil.getActiveCompaction(compactorAddr,
context).stream()
- .map(tac -> new ActiveCompactionImpl(context, tac,
compactorAddr,
- CompactionHost.Type.COMPACTOR))
- .collect(toList());
+ compactors.forEach((queue, compactorList) -> {
+ for (HostAndPort compactorAddr : compactorList) {
Review comment:
`queue` seems unused here. Why not just do:
```suggestion
for (HostAndPort compactorAddr : compactors.values()) {
```
##########
File path:
core/src/main/java/org/apache/accumulo/core/util/compaction/ExternalCompactionUtil.java
##########
@@ -51,6 +51,40 @@
public class ExternalCompactionUtil {
+ private static class RunningCompactionInformation {
+ private final String queue;
+ private final HostAndPort compactor;
+ private final Future<TExternalCompactionJob> future;
+
+ public RunningCompactionInformation(String queue, HostAndPort compactor,
+ Future<TExternalCompactionJob> future) {
+ super();
Review comment:
```suggestion
```
##########
File path: core/src/main/thrift/compaction-coordinator.thrift
##########
@@ -81,8 +94,7 @@ service CompactionCoordinatorService {
1:trace.TInfo tinfo
2:security.TCredentials credentials
3:string externalCompactionId
- 4:TCompactionState state
- 5:string message
+ 4:TCompactionStatusUpdate status
6:i64 timestamp
Review comment:
```suggestion
4:TCompactionStatusUpdate status
5:i64 timestamp
```
##########
File path:
server/compactor/src/main/java/org/apache/accumulo/compactor/Compactor.java
##########
@@ -513,7 +512,10 @@ public void run() {
Preconditions.checkState(compactionRunning.compareAndSet(false, true));
try {
LOG.info("Starting up compaction runnable for job: {}", job);
- updateCompactionState(job, TCompactionState.STARTED, "Compaction
started");
+ TCompactionStatusUpdate update = new TCompactionStatusUpdate();
+ update.setState(TCompactionState.STARTED);
+ update.setMessage("Compaction started");
+ updateCompactionState(job, update);
Review comment:
For thrift types, it's probably better to use the constructor that sets
all the fields. The no-arg constructor is primarily there for constructing the
object from the on-the-wire data. The advantage of avoiding these setters and
using the specific constructor that sets them all, is that if we change
something to add a new member variable for a thrift type, and regenerate the
Thrift classes, the constructor will change, and we'll be able to easily detect
it by observing the compiler errors. Using the no-arg constructor with the
setters, it's easier for us to fail to set a newly added parameter.
--
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]
