[GitHub] [accumulo] keith-turner commented on a change in pull request #2136: Remove redundant information in tablets external compaction entry

[GitBox <gi...@apache.org>]

keith-turner commented on a change in pull request #2136:
URL: https://github.com/apache/accumulo/pull/2136#discussion_r644299836



##########
File path: core/src/test/java/org/apache/accumulo/core/metadata/schema/TabletFileTest.java
##########
@@ -40,6 +41,16 @@ private TabletFile test(String metadataEntry, String volume, String tableId, Str
     return tabletFile;
   }
 
+  @Test
+  public void testEquivalence() {

Review comment:
       Could have a static function in DataFileManager that does the conversion and then have a unit test calls that function and verify the result.

##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/DatafileManager.java
##########
@@ -398,13 +398,19 @@ StoredTabletFile bringMinorCompactionOnline(TabletFile tmpDatafile, TabletFile n
   }
 
   StoredTabletFile bringMajorCompactionOnline(Set<StoredTabletFile> oldDatafiles,
-      TabletFile tmpDatafile, TabletFile newDatafile, Long compactionId,
-      Set<StoredTabletFile> selectedFiles, DataFileValue dfv, Optional<ExternalCompactionId> ecid)
-      throws IOException {
+      TabletFile tmpDatafile, Long compactionId, Set<StoredTabletFile> selectedFiles,
+      DataFileValue dfv, Optional<ExternalCompactionId> ecid) throws IOException {
     final KeyExtent extent = tablet.getExtent();
     VolumeManager vm = tablet.getTabletServer().getContext().getVolumeManager();
     long t1, t2;
 
+    int idx = tmpDatafile.getMetaInsert().indexOf("_tmp");
+    String newFilePath = tmpDatafile.getMetaInsert();
+    if (idx > 0) {
+      newFilePath = newFilePath.substring(0, idx);
+    }

Review comment:
       Should probably throw an exception in an else if `_tmp` suffix is not there, its expected to be there.

##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java
##########
@@ -973,14 +973,15 @@ public ExternalCompactionJob reserveExternalCompaction(CompactionServiceId servi
       Map<String,String> overrides =
           CompactableUtils.getOverrides(job.getKind(), tablet, cInfo.localHelper, job.getFiles());
 
-      var newFile = tablet.getNextMapFilename(!cInfo.propagateDeletes ? "A" : "C");
-      var compactTmpName = new TabletFile(new Path(newFile.getMetaInsert() + "_tmp"));
+      String tmpFileName =
+          tablet.getNextMapFilename(!cInfo.propagateDeletes ? "A" : "C").getMetaInsert() + "_tmp";
+      TabletFile compactTmpName = new TabletFile(new Path(tmpFileName));

Review comment:
       Could have a little static function in CompactableUtils that does this, then call that function here and in CompactableUtils.  That makes it easier for anyone looking at the code to find the two places where this happens.




-- 
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:
users@infra.apache.org