milleruntime commented on a change in pull request #2096:
URL: https://github.com/apache/accumulo/pull/2096#discussion_r632593672
##########
File path:
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java
##########
@@ -277,6 +378,130 @@ private void checkIfUserCompactionCanceled() {
}
}
+ private void initializeSelection(
+ Map<ExternalCompactionId,ExternalCompactionMetadata> extCompactions,
Tablet tablet,
+ Map<ExternalCompactionId,String> externalCompactionsToRemove) {
+ CompactionKind extKind = null;
+ boolean unexpectedExternal = false;
+ Set<StoredTabletFile> tmpSelectedFiles = null;
+ Boolean selAll = null;
+ Long cid = null;
+ Boolean propDel = null;
+ int count = 0;
+
+ ArrayList<String> reasons = new ArrayList<>();
+
+ for (Entry<ExternalCompactionId,ExternalCompactionMetadata> entry :
extCompactions.entrySet()) {
Review comment:
Breaking this method up into smaller methods would make it a lot easier
to grok. Perhaps one for the information gathering in this loop.
##########
File path:
core/src/main/java/org/apache/accumulo/core/metadata/schema/ExternalCompactionFinalState.java
##########
@@ -0,0 +1,131 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.metadata.schema;
+
+import java.util.Base64;
+
+import org.apache.accumulo.core.data.TableId;
+import org.apache.accumulo.core.dataImpl.KeyExtent;
+import org.apache.accumulo.core.util.TextUtil;
+import org.apache.hadoop.io.Text;
+
+import com.google.common.base.Preconditions;
+import com.google.gson.Gson;
+import com.google.gson.GsonBuilder;
+
+public class ExternalCompactionFinalState {
+
+ private static final Gson GSON = new GsonBuilder().create();
+
+ public enum FinalState {
+ FINISHED, FAILED
+ }
+
+ private ExternalCompactionId ecid;
+ private KeyExtent extent;
+ private FinalState state;
+ private long fileSize;
+ private long fileEntries;
+
+ public ExternalCompactionFinalState(ExternalCompactionId ecid, KeyExtent
extent, FinalState state,
+ long fileSize, long fileEntries) {
+ this.ecid = ecid;
+ this.extent = extent;
+ this.state = state;
+ this.fileSize = fileSize;
+ this.fileEntries = fileEntries;
+ }
+
+ public ExternalCompactionId getExternalCompactionId() {
+ return ecid;
+ }
+
+ public FinalState getFinalState() {
+ return state;
+ }
+
+ public KeyExtent getExtent() {
+ return extent;
+ }
+
+ public long getFileSize() {
+ Preconditions.checkState(state == FinalState.FINISHED);
+ return fileSize;
+ }
+
+ public long getEntries() {
+ Preconditions.checkState(state == FinalState.FINISHED);
+ return fileEntries;
+ }
+
+ private static class Extent {
+
+ String tableId;
+ String er;
+ String per;
+
+ Extent(KeyExtent extent) {
+ this.tableId = extent.tableId().canonical();
+ if (extent.endRow() != null) {
+ er =
Base64.getEncoder().encodeToString(TextUtil.getBytes(extent.endRow()));
+ }
+
+ if (extent.prevEndRow() != null) {
+ per =
Base64.getEncoder().encodeToString(TextUtil.getBytes(extent.prevEndRow()));
+ }
Review comment:
I think these could be final:
```suggestion
final String tableId;
final String er;
final String per;
Extent(KeyExtent extent) {
this.tableId = extent.tableId().canonical();
if (extent.endRow() != null) {
er =
Base64.getEncoder().encodeToString(TextUtil.getBytes(extent.endRow()));
} else {
er = null;
}
if (extent.prevEndRow() != null) {
per =
Base64.getEncoder().encodeToString(TextUtil.getBytes(extent.prevEndRow()));
} else {
per = null;
}
```
##########
File path:
core/src/main/java/org/apache/accumulo/core/spi/compaction/ExecutorManager.java
##########
@@ -31,4 +31,9 @@
* Create a thread pool executor within a compaction service.
*/
public CompactionExecutorId createExecutor(String name, int threads);
+
+ /**
+ * @return an id for a configured external execution queue.
+ */
+ public CompactionExecutorId getExternalExecutor(String name);
Review comment:
I don't think we need the `ExecutorManager` interface. These methods
could exist on the `InitParameters` interface. If a user wrote their own
`ExecutorManager` it doesn't look like there is a way for Accumulo to use it
since CompactionService is always calling the internal one.
##########
File path:
core/src/main/java/org/apache/accumulo/core/util/compaction/CompactionJobImpl.java
##########
@@ -40,14 +42,25 @@
private final Set<CompactableFile> files;
private final CompactionKind kind;
private boolean selectedAll;
+ private boolean hasSelectedAll;
- CompactionJobImpl(long priority, CompactionExecutorId executor,
Collection<CompactableFile> files,
- CompactionKind kind, boolean selectedAllFiles) {
+ public CompactionJobImpl(long priority, CompactionExecutorId executor,
+ Collection<CompactableFile> files, CompactionKind kind, boolean
selectedAllFiles) {
this.priority = priority;
this.executor = Objects.requireNonNull(executor);
this.files = Set.copyOf(files);
- this.kind = kind;
+ this.kind = Objects.requireNonNull(kind);
this.selectedAll = selectedAllFiles;
+ this.hasSelectedAll = true;
+ }
+
+ public CompactionJobImpl(long priority, CompactionExecutorId executor,
+ Collection<CompactableFile> files, CompactionKind kind) {
+ this.priority = priority;
+ this.executor = Objects.requireNonNull(executor);
+ this.files = Set.copyOf(files);
+ this.kind = Objects.requireNonNull(kind);
+ this.hasSelectedAll = false;
Review comment:
Looks like the new constructor is just to set `hasSelectedAll` to false,
throwing the error. There is another `selectedAll` in CompactableImpl that is
set as well but that one is Boolean object so could be null? It looks like this
`selectedAll` is purely to set `propogateDeletes` so maybe it could be renamed
to that? But in `ExternalCompactionMetadata` there are two booleans,
`selectAll` and `propogateDeletes`? Needless to say I'm confused and lost.
##########
File path:
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java
##########
@@ -539,54 +766,58 @@ public boolean isCompactionEnabled() {
}
}
- @Override
- public void compact(CompactionServiceId service, CompactionJob job,
RateLimiter readLimiter,
- RateLimiter writeLimiter, long queuedTime) {
-
- Set<StoredTabletFile> jobFiles = job.getFiles().stream()
- .map(cf -> ((CompactableFileImpl)
cf).getStoredTabletFile()).collect(Collectors.toSet());
-
- Long compactionId = null;
+ private static class CompactionInfo {
+ Set<StoredTabletFile> jobFiles;
Long checkCompactionId = null;
boolean propogateDeletes = true;
CompactionHelper localHelper;
List<IteratorSetting> iters = List.of();
CompactionConfig localCompactionCfg;
+ boolean selectedAll;
+ Set<StoredTabletFile> selectedFiles;
+ }
+
+ private CompactionInfo reserveFilesForCompaction(CompactionServiceId
service, CompactionJob job) {
Review comment:
Would be helpful to mention this method can return null and why. I am
not sure using `Optional` would help but it is a thought.
##########
File path:
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java
##########
@@ -95,6 +117,8 @@
private Supplier<Set<CompactionServiceId>> servicesInUse;
+ private Set<CompactionServiceId> servicesUsed = new
ConcurrentSkipListSet<>();
+
// status of special compactions
private enum SpecialStatus {
NEW, SELECTING, SELECTED, NOT_ACTIVE, CANCELED
Review comment:
This enum name is a bit obscure. It looks like its only used for the
status of file selection. Maybe `FileSelectionStatus` might make more sense
##########
File path:
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java
##########
@@ -277,6 +378,130 @@ private void checkIfUserCompactionCanceled() {
}
}
+ private void initializeSelection(
Review comment:
There is a lot of checking going on in this method. At least one comment
describing some high level goals would be very helpful.
##########
File path:
core/src/main/java/org/apache/accumulo/core/metadata/schema/ExternalCompactionFinalState.java
##########
@@ -0,0 +1,131 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.metadata.schema;
+
+import java.util.Base64;
+
+import org.apache.accumulo.core.data.TableId;
+import org.apache.accumulo.core.dataImpl.KeyExtent;
+import org.apache.accumulo.core.util.TextUtil;
+import org.apache.hadoop.io.Text;
+
+import com.google.common.base.Preconditions;
+import com.google.gson.Gson;
+import com.google.gson.GsonBuilder;
+
+public class ExternalCompactionFinalState {
+
+ private static final Gson GSON = new GsonBuilder().create();
+
+ public enum FinalState {
+ FINISHED, FAILED
+ }
+
+ private ExternalCompactionId ecid;
+ private KeyExtent extent;
+ private FinalState state;
+ private long fileSize;
+ private long fileEntries;
Review comment:
These fields could be final
```suggestion
private final ExternalCompactionId ecid;
private final KeyExtent extent;
private final FinalState state;
private final long fileSize;
private final long fileEntries;
```
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
