keith-turner commented on code in PR #4247:
URL: https://github.com/apache/accumulo/pull/4247#discussion_r1488707917
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/CompactionMetadata.java:
##########
@@ -121,7 +122,7 @@ public static CompactionMetadata fromJson(String json) {
return new
CompactionMetadata(jData.inputs.stream().map(StoredTabletFile::new).collect(toSet()),
StoredTabletFile.of(jData.tmp).getTabletFile(), jData.compactor,
CompactionKind.valueOf(jData.kind), jData.priority,
- CompactorGroupIdImpl.groupId(jData.groupId), jData.propDels,
jData.fateTxId);
+ CompactorGroupIdImpl.groupId(jData.groupId), jData.propDels,
jData.fateId);
Review Comment:
This suggestion goes with another comment.
```suggestion
CompactorGroupIdImpl.groupId(jData.groupId), jData.propDels,
FateId.from(jData.fateId));
```
##########
server/base/src/main/java/org/apache/accumulo/server/manager/state/TabletManagementParameters.java:
##########
@@ -205,7 +206,7 @@ private static class JsonData {
final Map<String,Set<String>> tserverGroups;
- final Map<Long,Map<String,String>> compactionHints;
+ final Map<FateId,Map<String,String>> compactionHints;
Review Comment:
Making the key type for this map a string that is the canonical value would
yield nicer and more stable json.
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/bulkVer2/CleanUpBulkImport.java:
##########
@@ -108,13 +108,12 @@ private static void removeBulkLoadEntries(Ample ample,
TableId tableId, FateId f
var tabletsMutator = ample.conditionallyMutateTablets()) {
for (var tablet : tablets) {
- // ELASTICITY_TODO DEFERRED - ISSUE 4044
- if (tablet.getLoaded().values().stream().anyMatch(l -> l ==
fateId.getTid())) {
+ if (tablet.getLoaded().values().stream()
+ .anyMatch(loadedFateId -> loadedFateId.equals(fateId))) {
var tabletMutator =
tabletsMutator.mutateTablet(tablet.getExtent()).requireAbsentOperation();
- tablet.getLoaded().entrySet().stream()
- .filter(entry -> entry.getValue() ==
fateId.getTid()).map(Map.Entry::getKey)
- .forEach(tabletMutator::deleteBulkFile);
+ tablet.getLoaded().entrySet().stream().filter(entry ->
entry.getValue().equals(fateId))
Review Comment:
Hard to tell from looking at this diff, but I assume the types line up?
Meaning `entry.getValue()` is a FateId type.
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/SelectedFiles.java:
##########
@@ -90,7 +90,7 @@ public SelectedFiles read(JsonReader in) throws IOException {
String name = in.nextName();
switch (name) {
case "txid":
Review Comment:
```suggestion
case "fateId":
```
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/SelectedFiles.java:
##########
@@ -43,26 +43,26 @@ public class SelectedFiles {
private final Set<StoredTabletFile> files;
private final boolean initiallySelectedAll;
- private final long fateTxId;
+ private final FateId fateId;
private String metadataValue;
private static final Gson GSON = new GsonBuilder()
.registerTypeAdapter(SelectedFiles.class, new
SelectedFilesTypeAdapter()).create();
- public SelectedFiles(Set<StoredTabletFile> files, boolean
initiallySelectedAll, long fateTxId) {
+ public SelectedFiles(Set<StoredTabletFile> files, boolean
initiallySelectedAll, FateId fateId) {
Preconditions.checkArgument(files != null && !files.isEmpty());
this.files = Set.copyOf(files);
this.initiallySelectedAll = initiallySelectedAll;
- this.fateTxId = fateTxId;
+ this.fateId = fateId;
}
private static class SelectedFilesTypeAdapter extends
TypeAdapter<SelectedFiles> {
@Override
public void write(JsonWriter out, SelectedFiles selectedFiles) throws
IOException {
out.beginObject();
- out.name("txid").value(FateTxId.formatTid(selectedFiles.getFateTxId()));
+ out.name("txid").value(selectedFiles.getFateId().canonical());
Review Comment:
```suggestion
out.name("fateId").value(selectedFiles.getFateId().canonical());
```
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/MetadataSchema.java:
##########
@@ -334,18 +334,13 @@ public static class BulkFileColumnFamily {
public static final String STR_NAME = "loaded";
public static final Text NAME = new Text(STR_NAME);
- public static long getBulkLoadTid(Value v) {
+ public static FateId getBulkLoadTid(Value v) {
return getBulkLoadTid(v.toString());
}
- public static long getBulkLoadTid(String vs) {
- if (FateTxId.isFormatedTid(vs)) {
- return FateTxId.fromString(vs);
- } else {
- // a new serialization format was introduce in 2.0. This code
support deserializing the
- // old format.
- return Long.parseLong(vs);
- }
+ public static FateId getBulkLoadTid(String vs) {
+ // ELASTICITY_TODO issue 4044 will this be an issue removing the old
code?
Review Comment:
```suggestion
// ELASTICITY_TODO issue 4044 will this be an issue removing the old
code? May need to introduce code in upgrade to handle old format.
```
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/CompactionMetadata.java:
##########
@@ -111,7 +112,7 @@ public String toJson() {
jData.groupId = cgid.toString();
jData.priority = priority;
jData.propDels = propagateDeletes;
- jData.fateTxId = fateTxId;
+ jData.fateId = fateId;
Review Comment:
The following would probably make mch more readable json and be tolerant of
changes to the implementation of FateId like variable renames.
```suggestion
jData.fateId = fateId.canonical();
```
##########
core/src/main/java/org/apache/accumulo/core/fate/FateId.java:
##########
@@ -34,6 +34,8 @@ public class FateId extends AbstractId<FateId> {
private static final long serialVersionUID = 1L;
private static final String PREFIX = "FATE:";
private static final Pattern HEX_PATTERN = Pattern.compile("^[0-9a-fA-F]+$");
+ private static final Pattern FATEID_PATTERN =
+ Pattern.compile("^" + PREFIX + "[a-zA-Z]+:[0-9a-fA-F]+$");
Review Comment:
Not sure if the regex below is correct or if the code compiles. The code
below attempts to create a regex like `"^" + PREFIX +
"(META|USER):[0-9a-fA-F]+$`. If this works it would simplify the validation
code, no need to have the checks of the fate instance type.
```suggestion
Pattern.compile("^" + PREFIX +
"("+Stream.of(FateInstanceType.values()).map(fit->fit.name()).collect(Collectors.joining("|"))+"):[0-9a-fA-F]+$");
```
##########
core/src/main/java/org/apache/accumulo/core/fate/FateId.java:
##########
@@ -86,6 +88,46 @@ public static FateId from(FateInstanceType type, String
hexTid) {
}
}
+ /**
+ * @param fateIdStr the string representation of the FateId
+ * @return a new FateId object from the given string
+ */
+ public static FateId from(String fateIdStr) {
+ if (FATEID_PATTERN.matcher(fateIdStr).matches()) {
+ String[] fields = fateIdStr.split(":");
Review Comment:
If the regex that includes the FatenstanceTpe mentioned in the other comment
works the could remove this code that validates the FateInstanceType
##########
server/base/src/main/java/org/apache/accumulo/server/compaction/CompactionConfigStorage.java:
##########
@@ -83,29 +85,30 @@ public static CompactionConfig getConfig(ServerContext
context, long fateTxId,
}
}
- public static void setConfig(ServerContext context, long fateTxId, byte[]
encConfig)
+ public static void setConfig(ServerContext context, FateId fateId, byte[]
encConfig)
throws InterruptedException, KeeperException {
- context.getZooReaderWriter().putPrivatePersistentData(createPath(context,
fateTxId), encConfig,
+ context.getZooReaderWriter().putPrivatePersistentData(createPath(context,
fateId), encConfig,
ZooUtil.NodeExistsPolicy.SKIP);
}
- public static void deleteConfig(ServerContext context, long fateTxId)
+ public static void deleteConfig(ServerContext context, FateId fateId)
throws InterruptedException, KeeperException {
- context.getZooReaderWriter().delete(createPath(context, fateTxId));
+ context.getZooReaderWriter().delete(createPath(context, fateId));
}
- public static Map<Long,CompactionConfig> getAllConfig(ServerContext context,
+ public static Map<FateId,CompactionConfig> getAllConfig(ServerContext
context,
Predicate<TableId> tableIdPredicate) throws InterruptedException,
KeeperException {
- Map<Long,CompactionConfig> configs = new HashMap<>();
+ Map<FateId,CompactionConfig> configs = new HashMap<>();
var children = context.getZooReaderWriter()
.getChildren(context.getZooKeeperRoot() + Constants.ZCOMPACTIONS);
for (var child : children) {
- var fateTxid = Long.parseLong(child, 16);
- var cconf = getConfig(context, fateTxid, tableIdPredicate);
+ String[] fields = child.split(DELIMITER);
Review Comment:
```suggestion
String[] fields = child.split(DELIMITER);
Preconditions.checkState(fields.length == 2, "Unexpected child %s",
child);
```
##########
test/src/main/java/org/apache/accumulo/test/functional/SplitRecoveryIT.java:
##########
@@ -46,6 +46,8 @@
import org.apache.accumulo.core.data.TableId;
import org.apache.accumulo.core.data.Value;
import org.apache.accumulo.core.dataImpl.KeyExtent;
+import org.apache.accumulo.core.fate.FateId;
+import org.apache.accumulo.core.fate.FateInstanceType;
Review Comment:
#4260 will conflict with these changes, will hold off on merging it until
this PR is merged.
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/CompactionMetadata.java:
##########
@@ -98,7 +99,7 @@ private static class GSonData {
String groupId;
short priority;
boolean propDels;
- Long fateTxId;
Review Comment:
We may need a follow on update for upgrade
##########
test/src/main/java/org/apache/accumulo/test/functional/AmpleConditionalWriterIT.java:
##########
@@ -745,58 +751,70 @@ public void testCompacted() {
var ctmi = new ConditionalTabletsMutatorImpl(context);
+ FateInstanceType type = FateInstanceType.fromTableId(tid);
+ FateId fateId55L = FateId.from(type, 55L);
+ FateId fateId45L = FateId.from(type, 45L);
+ FateId fateId56L = FateId.from(type, 56L);
+ FateId fateId65L = FateId.from(type, 65L);
+ FateId fateId75L = FateId.from(type, 75L);
Review Comment:
Reordered the vars to be sorted.
```suggestion
FateId fateId45L = FateId.from(type, 45L);
FateId fateId55L = FateId.from(type, 55L);
FateId fateId56L = FateId.from(type, 56L);
FateId fateId65L = FateId.from(type, 65L);
FateId fateId75L = FateId.from(type, 75L);
```
--
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]
