From 71cb74b3bf77917929cfa78a662a75f683681d3f Mon Sep 17 00:00:00 2001
From: "Ariel Shaqed (Scolnicov)" <ariels@treeverse.io>
Date: Sun, 24 Nov 2024 16:42:23 +0200
Subject: [PATCH] Simplify PhysicalAddressCreator selection

---
 .../lakefs/LakeFSFileSystemServerS3Test.java  | 20 +++++++++----------
 .../src/test/java/io/lakefs/S3FSTestBase.java |  9 +++------
 2 files changed, 12 insertions(+), 17 deletions(-)

diff --git a/clients/hadoopfs/src/test/java/io/lakefs/LakeFSFileSystemServerS3Test.java b/clients/hadoopfs/src/test/java/io/lakefs/LakeFSFileSystemServerS3Test.java
index 3e2c50f2eb1..55efc2bb2a4 100644
--- a/clients/hadoopfs/src/test/java/io/lakefs/LakeFSFileSystemServerS3Test.java
+++ b/clients/hadoopfs/src/test/java/io/lakefs/LakeFSFileSystemServerS3Test.java
@@ -42,17 +42,15 @@ public static Iterable<Object[]> data() {
                 {new PresignedPhysicalAddressCreator(), "presigned"}});
     }
 
+    @Parameter(0)
+    public PhysicalAddressCreator pac;
+
     @Parameter(1)
     public String unusedAddressCreatorType;
 
-    @Parameter(0)
-    public PhysicalAddressCreator pacParam;
-
     @Override
-    // pac is defined in a superclass but parametrized here.
-    protected void setPac() {
-        pac = pacParam;
-        LOG.info("Set pac!");
+    protected PhysicalAddressCreator getPac() {
+        return pac;
     }
 
     @Test
@@ -115,7 +113,7 @@ public void testMkdirs() throws IOException {
             mockGetPhysicalAddress("repo", "main", "dir1/dir2/dir3/", "repo-base/emptyDir");
 
         ObjectStats newStats = makeObjectStats("dir1/dir2/dir3/")
-            .physicalAddress(pac.createGetPhysicalAddress(this, "repo-base/dir12"));
+            .physicalAddress(getPac().createGetPhysicalAddress(this, "repo-base/dir12"));
         mockStatObject("repo", "main", "dir1/dir2/dir3/", newStats);
 
         mockServerClient.when(request()
@@ -144,7 +142,7 @@ public void testCreateExistingDirectory() throws IOException {
 
         mockStatObjectNotFound("repo", "main", "sub1/sub2/create.me");
         ObjectStats stats = makeObjectStats("sub1/sub2/create.me/")
-            .physicalAddress(pac.createGetPhysicalAddress(this, "repo-base/sub1/sub2/create.me"));
+            .physicalAddress(getPac().createGetPhysicalAddress(this, "repo-base/sub1/sub2/create.me"));
         mockStatObject("repo", "main", "sub1/sub2/create.me/", stats);
 
         Exception e =
@@ -169,7 +167,7 @@ public void testOpen() throws IOException, ApiException {
         String contents = "The quick brown fox jumps over the lazy dog.";
         byte[] contentsBytes = contents.getBytes();
         String physicalPath = sessionId() + "/repo-base/open";
-        String physicalKey = pac.createGetPhysicalAddress(this, physicalPath);
+        String physicalKey = getPac().createGetPhysicalAddress(this, physicalPath);
         int readBufferSize = 5;
         Path path = new Path("lakefs://repo/main/read.me");
 
@@ -220,7 +218,7 @@ public void testOpenWithInvalidUriChars() throws IOException, ApiException {
 
             String path = String.format("lakefs://repo/main/%s-x", suffix);
             ObjectStats stats = makeObjectStats(suffix + "-x")
-                .physicalAddress(pac.createGetPhysicalAddress(this, key))
+                .physicalAddress(getPac().createGetPhysicalAddress(this, key))
                 .sizeBytes((long) contentsBytes.length);
             mockStatObject("repo", "main", suffix + "-x", stats);
 
diff --git a/clients/hadoopfs/src/test/java/io/lakefs/S3FSTestBase.java b/clients/hadoopfs/src/test/java/io/lakefs/S3FSTestBase.java
index 1a97bb39f08..79dae41a81d 100644
--- a/clients/hadoopfs/src/test/java/io/lakefs/S3FSTestBase.java
+++ b/clients/hadoopfs/src/test/java/io/lakefs/S3FSTestBase.java
@@ -44,8 +44,6 @@
 public abstract class S3FSTestBase extends FSTestBase {
     static private final Logger LOG = LoggerFactory.getLogger(S3FSTestBase.class);
 
-    protected PhysicalAddressCreator pac = new SimplePhysicalAddressCreator();
-
     protected String s3Endpoint;
     protected AmazonS3 s3Client;
 
@@ -127,10 +125,9 @@ protected void assertS3Object(StagingLocation stagingLocation, String contents)
         }
     }
 
-    protected void setPac() {}
+    protected abstract PhysicalAddressCreator getPac();
 
     protected void moreHadoopSetup() {
-        setPac();
         s3ClientSetup();
 
         conf.set("fs.s3a.impl", "org.apache.hadoop.fs.s3a.S3AFileSystem");
@@ -138,7 +135,7 @@ protected void moreHadoopSetup() {
         conf.set(org.apache.hadoop.fs.s3a.Constants.SECRET_KEY, S3_SECRET_ACCESS_KEY);
         conf.set(org.apache.hadoop.fs.s3a.Constants.ENDPOINT, s3Endpoint);
         conf.set(org.apache.hadoop.fs.s3a.Constants.BUFFER_DIR, "/tmp/s3a");
-        pac.initConfiguration(conf);
+        getPac().initConfiguration(conf);
 
         LOG.info("Setup done!");
     }
@@ -196,7 +193,7 @@ public StagingLocation createPutStagingLocation(S3FSTestBase o, String namespace
     // Return a location under namespace for this getPhysicalAddress call.
     protected StagingLocation mockGetPhysicalAddress(String repo, String branch, String path, String namespace) {
         StagingLocation stagingLocation =
-            pac.createPutStagingLocation(this, namespace, repo, branch, path);
+            getPac().createPutStagingLocation(this, namespace, repo, branch, path);
         mockServerClient.when(request()
                               .withMethod("GET")
                               .withPath(String.format("/repositories/%s/branches/%s/staging/backing", repo, branch))