[GitHub] [accumulo] keith-turner commented on a diff in pull request #3417: Refactor StoredTabletFile to contain a TabletFile

["keith-turner (via GitHub)" <gi...@apache.org>]

keith-turner commented on code in PR #3417:
URL: https://github.com/apache/accumulo/pull/3417#discussion_r1206152354


##########
core/src/main/java/org/apache/accumulo/core/logging/TabletLogger.java:
##########
@@ -140,7 +140,7 @@ public static void compacting(KeyExtent extent, CompactionJob job, CompactionCon
     }
   }
 
-  public static void compacted(KeyExtent extent, CompactionJob job, TabletFile output) {
+  public static void compacted(KeyExtent extent, CompactionJob job, AbstractTabletFile<?> output) {

Review Comment:
   Could this be StoredTabletFile?  Looking at the two places that called it it seems StoredTabletFile was passed.



##########
server/base/src/main/java/org/apache/accumulo/server/util/ManagerMetadataUtil.java:
##########
@@ -199,10 +199,10 @@ public static void replaceDatafiles(ServerContext context, KeyExtent extent,
     TabletMutator tablet = context.getAmple().mutateTablet(extent);
 
     datafilesToDelete.forEach(tablet::deleteFile);
-    scanFiles.forEach(tablet::putScan);
+    scanFiles.stream().map(StoredTabletFile::getTabletFile).forEach(tablet::putScan);

Review Comment:
   Wondering if putScan needs an overloaded method, not sure if its ever updating existing metadata.



##########
core/src/main/java/org/apache/accumulo/core/metadata/StoredTabletFile.java:
##########
@@ -60,6 +65,22 @@ public Text getMetaUpdateDeleteText() {
     return new Text(getMetaUpdateDelete());
   }
 
+  public TabletFile getTabletFile() {
+    return tabletFile;
+  }
+
+  public TableId getTableId() {
+    return tabletFile.getTableId();
+  }
+
+  public String getPathStr() {
+    return tabletFile.getPathStr();
+  }
+
+  public Text getMetaInsertText() {
+    return tabletFile.getMetaInsertText();
+  }

Review Comment:
   I think this method should be deleted because it normalizes the path. Its only used in one place and where its used we are updating existing files in the metadata table, so do not want to normalize.
   
   ```suggestion
   ```



##########
server/base/src/main/java/org/apache/accumulo/server/util/ManagerMetadataUtil.java:
##########
@@ -199,10 +199,10 @@ public static void replaceDatafiles(ServerContext context, KeyExtent extent,
     TabletMutator tablet = context.getAmple().mutateTablet(extent);
 
     datafilesToDelete.forEach(tablet::deleteFile);
-    scanFiles.forEach(tablet::putScan);
+    scanFiles.stream().map(StoredTabletFile::getTabletFile).forEach(tablet::putScan);
 
     if (path.isPresent()) {
-      tablet.putFile(path.orElseThrow(), size);
+      tablet.putFile(path.orElseThrow().getTabletFile(), size);

Review Comment:
   For the putFile method on [TabletMutator](https://github.com/apache/accumulo/blob/0017fd8949cc1bc7253b8e90313d4fd1b71e2241/server/base/src/main/java/org/apache/accumulo/server/metadata/TabletMutatorBase.java#L84) I think it would best to add an overloaded version of the method that takes a StoredTabletFile (keeping the one that takes a TabletFile if needed). Then if the code has a StoredTabletFile it can pass it.  The putFile() method is called to add new files and to update the DataFileValue for existing files.  For the case of updating existing files need to pass the StoredTabletFile through instead of converting to the a TabletFile (which normalizes).



##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/bulkVer2/LoadFiles.java:
##########
@@ -219,7 +221,8 @@ void load(List<TabletMetadata> tablets, Files files) {
           server = location.getHostAndPort();
         }
 
-        Set<TabletFile> loadedFiles = tablet.getLoaded().keySet();
+        Set<TabletFile> loadedFiles = tablet.getLoaded().keySet().stream()

Review Comment:
   Before this change, this code it may have been comparing StoredTabletFile to TabletFile later in the code.  I think this was a place that was relying on the sketchy hashCode and equals 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: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org