keith-turner commented on code in PR #4437:
URL: https://github.com/apache/accumulo/pull/4437#discussion_r1554648985
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java:
##########
@@ -556,4 +557,158 @@ private static Optional<TServerInstance>
checkServer(ClientContext context, Stri
.map(sld -> sld.getAddress(ServiceLockData.ThriftService.TSERV))
.map(address -> new TServerInstance(address,
stat.getEphemeralOwner()));
}
+
+ static class Builder {
+ private TableId tableId;
+ private Text prevEndRow;
+ private boolean sawPrevEndRow;
+ private Text oldPrevEndRow;
+ private boolean sawOldPrevEndRow;
+ private Text endRow;
+ private Location location;
+ private final ImmutableMap.Builder<StoredTabletFile,DataFileValue> files =
+ ImmutableMap.builder();
+ private final ImmutableList.Builder<StoredTabletFile> scans =
ImmutableList.builder();
+ private final ImmutableMap.Builder<StoredTabletFile,Long> loadedFiles =
ImmutableMap.builder();
+ private EnumSet<ColumnType> fetchedCols;
+ private Location last;
+ private SuspendingTServer suspend;
+ private String dirName;
+ private MetadataTime time;
+ private String cloned;
+ private ImmutableSortedMap.Builder<Key,Value> keyValues;
+ private OptionalLong flush = OptionalLong.empty();
+ private final ImmutableList.Builder<LogEntry> logs =
ImmutableList.builder();
+ private OptionalLong compact = OptionalLong.empty();
+ private Double splitRatio = null;
+ private final ImmutableMap.Builder<ExternalCompactionId,
+ ExternalCompactionMetadata> extCompactions = ImmutableMap.builder();
+ private boolean merged;
+
+ Builder table(TableId tableId) {
+ this.tableId = tableId;
+ return this;
+ }
Review Comment:
I only saw one use of chaining the builder. I think we could remove that
once instance of chaining and make all the methods have a void return type.
This would simplify the code a bit and may avoid uneeded runtime work.
```suggestion
void table(TableId tableId) {
this.tableId = tableId;
}
```
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java:
##########
@@ -442,88 +463,68 @@ public static <E extends Entry<Key,Value>> TabletMetadata
convertRow(Iterator<E>
case DIRECTORY_QUAL:
Preconditions.checkArgument(ServerColumnFamily.isValidDirCol(val),
"Saw invalid dir name %s %s", key, val);
- te.dirName = val;
+ tmBuilder.dirName(val);
break;
case TIME_QUAL:
- te.time = MetadataTime.parse(val);
+ tmBuilder.time(MetadataTime.parse(val));
break;
case FLUSH_QUAL:
- te.flush = OptionalLong.of(Long.parseLong(val));
+ tmBuilder.flush(Long.parseLong(val));
break;
case COMPACT_QUAL:
- te.compact = OptionalLong.of(Long.parseLong(val));
+ tmBuilder.compact(Long.parseLong(val));
break;
}
break;
case DataFileColumnFamily.STR_NAME:
- filesBuilder.put(new StoredTabletFile(qual), new DataFileValue(val));
+ tmBuilder.file(new StoredTabletFile(qual), new DataFileValue(val));
break;
case BulkFileColumnFamily.STR_NAME:
- loadedFilesBuilder.put(new StoredTabletFile(qual),
+ tmBuilder.loadedFile(new StoredTabletFile(qual),
BulkFileColumnFamily.getBulkLoadTid(val));
break;
case CurrentLocationColumnFamily.STR_NAME:
- te.setLocationOnce(val, qual, LocationType.CURRENT);
+ tmBuilder.location(val, qual, LocationType.CURRENT);
break;
case FutureLocationColumnFamily.STR_NAME:
- te.setLocationOnce(val, qual, LocationType.FUTURE);
+ tmBuilder.location(val, qual, LocationType.FUTURE);
break;
case LastLocationColumnFamily.STR_NAME:
- te.last = Location.last(val, qual);
+ tmBuilder.last(Location.last(val, qual));
break;
case SuspendLocationColumn.STR_NAME:
- te.suspend = SuspendingTServer.fromValue(kv.getValue());
+ tmBuilder.suspend(SuspendingTServer.fromValue(kv.getValue()));
break;
case ScanFileColumnFamily.STR_NAME:
- scansBuilder.add(new StoredTabletFile(qual));
+ tmBuilder.scan(new StoredTabletFile(qual));
break;
case ClonedColumnFamily.STR_NAME:
- te.cloned = val;
+ tmBuilder.cloned(val);
break;
case LogColumnFamily.STR_NAME:
- logsBuilder.add(LogEntry.fromMetaWalEntry(kv));
+ tmBuilder.log(LogEntry.fromMetaWalEntry(kv));
break;
case ExternalCompactionColumnFamily.STR_NAME:
- extCompBuilder.put(ExternalCompactionId.of(qual),
+ tmBuilder.extCompaction(ExternalCompactionId.of(qual),
ExternalCompactionMetadata.fromJson(val));
break;
case MergedColumnFamily.STR_NAME:
- te.merged = true;
+ tmBuilder.merged(true);
break;
default:
throw new IllegalStateException("Unexpected family " + fam);
}
}
- te.files = filesBuilder.build();
- te.loadedFiles = loadedFilesBuilder.build();
- te.fetchedCols = fetchedColumns;
- te.scans = scansBuilder.build();
- te.logs = logsBuilder.build();
- te.extCompactions = extCompBuilder.build();
- if (buildKeyValueMap) {
- te.keyValues = kvBuilder.build();
- }
- return te;
- }
-
- private void setLocationOnce(String val, String qual, LocationType lt) {
- if (location != null) {
- throw new IllegalStateException("Attempted to set second location for
tableId: " + tableId
- + " endrow: " + endRow + " -- " + location + " " + qual + " " + val);
- }
- location = new Location(val, qual, lt);
+ return tmBuilder.fetchedCols(fetchedColumns).build();
Review Comment:
If making all the builder methods have a void return type then could
possibly do the following.
```suggestion
return tmBuilder.build(fetchedColumns);
```
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java:
##########
@@ -556,4 +557,158 @@ private static Optional<TServerInstance>
checkServer(ClientContext context, Stri
.map(sld -> sld.getAddress(ServiceLockData.ThriftService.TSERV))
.map(address -> new TServerInstance(address,
stat.getEphemeralOwner()));
}
+
+ static class Builder {
+ private TableId tableId;
+ private Text prevEndRow;
+ private boolean sawPrevEndRow;
+ private Text oldPrevEndRow;
+ private boolean sawOldPrevEndRow;
+ private Text endRow;
+ private Location location;
+ private final ImmutableMap.Builder<StoredTabletFile,DataFileValue> files =
+ ImmutableMap.builder();
+ private final ImmutableList.Builder<StoredTabletFile> scans =
ImmutableList.builder();
+ private final ImmutableMap.Builder<StoredTabletFile,Long> loadedFiles =
ImmutableMap.builder();
+ private EnumSet<ColumnType> fetchedCols;
+ private Location last;
+ private SuspendingTServer suspend;
+ private String dirName;
+ private MetadataTime time;
+ private String cloned;
+ private ImmutableSortedMap.Builder<Key,Value> keyValues;
+ private OptionalLong flush = OptionalLong.empty();
+ private final ImmutableList.Builder<LogEntry> logs =
ImmutableList.builder();
+ private OptionalLong compact = OptionalLong.empty();
+ private Double splitRatio = null;
+ private final ImmutableMap.Builder<ExternalCompactionId,
+ ExternalCompactionMetadata> extCompactions = ImmutableMap.builder();
+ private boolean merged;
+
+ Builder table(TableId tableId) {
+ this.tableId = tableId;
+ return this;
+ }
+
+ Builder endRow(Text endRow) {
+ this.endRow = endRow;
+ return this;
+ }
+
+ Builder prevEndRow(Text prevEndRow) {
+ this.prevEndRow = prevEndRow;
+ return this;
+ }
+
+ Builder sawPrevEndRow(boolean sawPrevEndRow) {
+ this.sawPrevEndRow = sawPrevEndRow;
+ return this;
+ }
+
+ Builder oldPrevEndRow(Text oldPrevEndRow) {
+ this.oldPrevEndRow = oldPrevEndRow;
+ return this;
+ }
+
+ Builder sawOldPrevEndRow(boolean sawOldPrevEndRow) {
+ this.sawOldPrevEndRow = sawOldPrevEndRow;
+ return this;
+ }
+
+ Builder splitRatio(Double splitRatio) {
+ this.splitRatio = splitRatio;
+ return this;
+ }
+
+ Builder dirName(String dirName) {
+ this.dirName = dirName;
+ return this;
+ }
+
+ Builder time(MetadataTime time) {
+ this.time = time;
+ return this;
+ }
+
+ Builder flush(long flush) {
+ this.flush = OptionalLong.of(flush);
+ return this;
+ }
+
+ Builder compact(long compact) {
+ this.compact = OptionalLong.of(compact);
+ return this;
+ }
+
+ Builder file(StoredTabletFile stf, DataFileValue dfv) {
+ this.files.put(stf, dfv);
+ return this;
+ }
+
+ Builder loadedFile(StoredTabletFile stf, Long tid) {
+ this.loadedFiles.put(stf, tid);
+ return this;
+ }
+
+ Builder location(String val, String qual, LocationType lt) {
+ if (location != null) {
+ throw new IllegalStateException("Attempted to set second location for
tableId: " + tableId
Review Comment:
The builder pattern cleans up handing this situation.
--
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]
