You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "dlmarion (via GitHub)" <gi...@apache.org> on 2023/05/09 16:51:41 UTC
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3385: Encapsulate file metadata inside a new object
dlmarion commented on code in PR #3385:
URL: https://github.com/apache/accumulo/pull/3385#discussion_r1188850393
##########
core/src/main/java/org/apache/accumulo/core/gc/ReferenceDirectory.java:
##########
@@ -46,8 +47,8 @@ public String getTabletDir() {
* A Tablet directory should have a metadata entry equal to the dirName.
*/
@Override
- public String getMetadataEntry() {
- if (!tabletDir.equals(metadataEntry)) {
+ public TabletFileMetadataEntry getMetadataEntry() {
+ if (!tabletDir.equals(metadataEntry.getFilePathString())) {
Review Comment:
Since the variables are `final`, I think this check should be done in the constructor.
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/ExternalCompactionMetadata.java:
##########
@@ -129,10 +129,11 @@ private static class GSonData {
public String toJson() {
GSonData jData = new GSonData();
- jData.inputs = jobFiles.stream().map(StoredTabletFile::getMetaUpdateDelete).collect(toList());
- jData.nextFiles =
- nextFiles.stream().map(StoredTabletFile::getMetaUpdateDelete).collect(toList());
- jData.tmp = compactTmpName.getMetaInsert();
+ jData.inputs = jobFiles.stream()
+ .map(tabletFile -> tabletFile.getMetaUpdateDelete().getMetaString()).collect(toList());
Review Comment:
This breaks when/if `getMetaString` changes to include the file range.
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/ExternalCompactionMetadata.java:
##########
@@ -129,10 +129,11 @@ private static class GSonData {
public String toJson() {
GSonData jData = new GSonData();
- jData.inputs = jobFiles.stream().map(StoredTabletFile::getMetaUpdateDelete).collect(toList());
- jData.nextFiles =
- nextFiles.stream().map(StoredTabletFile::getMetaUpdateDelete).collect(toList());
- jData.tmp = compactTmpName.getMetaInsert();
+ jData.inputs = jobFiles.stream()
+ .map(tabletFile -> tabletFile.getMetaUpdateDelete().getMetaString()).collect(toList());
+ jData.nextFiles = nextFiles.stream()
+ .map(tabletFile -> tabletFile.getMetaUpdateDelete().getMetaString()).collect(toList());
Review Comment:
This breaks when/if `getMetaString` changes to include the file range.
##########
core/src/main/java/org/apache/accumulo/core/metadata/StoredTabletFile.java:
##########
@@ -33,39 +35,47 @@
* in Upgrader9to10.upgradeRelativePaths()
*/
public class StoredTabletFile extends TabletFile {
- private final String metadataEntry;
+ private final TabletFileMetadataEntry metadataEntry;
Review Comment:
Is this variable a duplicate of `TabletFile.normalizedPath`?
##########
core/src/main/java/org/apache/accumulo/core/metadata/StoredTabletFile.java:
##########
@@ -33,39 +35,47 @@
* in Upgrader9to10.upgradeRelativePaths()
*/
public class StoredTabletFile extends TabletFile {
- private final String metadataEntry;
+ private final TabletFileMetadataEntry metadataEntry;
+
+ /**
+ * Construct a tablet file using the {@link TabletFileMetadataEntry} from the metadata. Preserve
+ * the exact string so the entry can be deleted.
+ */
+ public StoredTabletFile(final TabletFileMetadataEntry metadataEntry) {
+ super(Objects.requireNonNull(metadataEntry).getFilePath());
+ this.metadataEntry = metadataEntry;
+ }
/**
* Construct a tablet file using the string read from the metadata. Preserve the exact string so
* the entry can be deleted.
*/
- public StoredTabletFile(String metadataEntry) {
- super(new Path(metadataEntry));
- this.metadataEntry = metadataEntry;
+ public StoredTabletFile(final String metadataEntry) {
+ this(TabletFileMetadataEntry.of(metadataEntry));
}
/**
- * Return the exact string that is stored in the metadata table. This is important for updating
- * and deleting metadata entries. If the exact string is not used, erroneous entries can pollute
- * the metadata table.
+ * Return the {@link TabletFileMetadataEntry} which contains the exact string that is stored in
+ * the metadata table. This is important for updating and deleting metadata entries. If the exact
+ * string is not used, erroneous entries can pollute the metadata table.
*/
- public String getMetaUpdateDelete() {
+ public TabletFileMetadataEntry getMetaUpdateDelete() {
return metadataEntry;
}
/**
* Return a new Text object of {@link #getMetaUpdateDelete()}
*/
public Text getMetaUpdateDeleteText() {
- return new Text(getMetaUpdateDelete());
+ return getMetaUpdateDelete().getMetaText();
Review Comment:
there is no difference between this method and `TabletFile.getMetaInsert`.
##########
server/base/src/main/java/org/apache/accumulo/server/compaction/FileCompactor.java:
##########
@@ -345,16 +345,16 @@ public CompactionStats call() throws IOException, CompactionCanceledException {
readers.add(reader);
InterruptibleIterator iter = new ProblemReportingIterator(context, extent.tableId(),
- mapFile.getPathStr(), false, reader);
+ mapFile.getPathStr().toString(), false, reader);
iter = filesToCompact.get(mapFile).wrapFileIterator(iter);
iters.add(iter);
} catch (Exception e) {
- ProblemReports.getInstance(context).report(
- new ProblemReport(extent.tableId(), ProblemType.FILE_READ, mapFile.getPathStr(), e));
+ ProblemReports.getInstance(context).report(new ProblemReport(extent.tableId(),
+ ProblemType.FILE_READ, mapFile.getPathStr().toString(), e));
Review Comment:
`getPathStr` is already a String.
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/ExternalCompactionMetadata.java:
##########
@@ -129,10 +129,11 @@ private static class GSonData {
public String toJson() {
GSonData jData = new GSonData();
- jData.inputs = jobFiles.stream().map(StoredTabletFile::getMetaUpdateDelete).collect(toList());
- jData.nextFiles =
- nextFiles.stream().map(StoredTabletFile::getMetaUpdateDelete).collect(toList());
- jData.tmp = compactTmpName.getMetaInsert();
+ jData.inputs = jobFiles.stream()
+ .map(tabletFile -> tabletFile.getMetaUpdateDelete().getMetaString()).collect(toList());
+ jData.nextFiles = nextFiles.stream()
+ .map(tabletFile -> tabletFile.getMetaUpdateDelete().getMetaString()).collect(toList());
+ jData.tmp = compactTmpName.getMetaInsert().getMetaString();
Review Comment:
This breaks when/if `getMetaString` changes to include the file range.
##########
server/base/src/main/java/org/apache/accumulo/server/compaction/FileCompactor.java:
##########
@@ -345,16 +345,16 @@ public CompactionStats call() throws IOException, CompactionCanceledException {
readers.add(reader);
InterruptibleIterator iter = new ProblemReportingIterator(context, extent.tableId(),
- mapFile.getPathStr(), false, reader);
+ mapFile.getPathStr().toString(), false, reader);
Review Comment:
`getPathStr` is already a String.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org