[GitHub] [iceberg] amogh-jahagirdar commented on a diff in pull request #7353: Core, Spark: Make ObjectStoreLocationProvider serializable

["amogh-jahagirdar (via GitHub)" <gi...@apache.org>]

amogh-jahagirdar commented on code in PR #7353:
URL: https://github.com/apache/iceberg/pull/7353#discussion_r1167668817


##########
core/src/main/java/org/apache/iceberg/LocationProviders.java:
##########
@@ -109,7 +109,7 @@ static class ObjectStoreLocationProvider implements LocationProvider {
 
     private static final HashFunction HASH_FUNC = Hashing.murmur3_32_fixed();
     private static final BaseEncoding BASE64_ENCODER = BaseEncoding.base64Url().omitPadding();
-    private final ThreadLocal<byte[]> temp = ThreadLocal.withInitial(() -> new byte[4]);
+    private transient ThreadLocal<byte[]> temp;

Review Comment:
   I think we can have a `static` threadlocal and keep the initialization as is (and remove 175-177) to keep the change a bit simpler?
    Static fields (like transient) are also not serialized. Let me know if I'm missing something



##########
spark/v3.3/spark/src/test/java/org/apache/iceberg/TestTableSerialization.java:
##########
@@ -33,11 +33,25 @@
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
 
+@RunWith(Parameterized.class)
 public class TestTableSerialization {
 
+  public TestTableSerialization(String isObjectStoreEnabled) {
+    this.isObjectStoreEnabled = isObjectStoreEnabled;
+  }
+
+  @Parameterized.Parameters(name = "isObjectStoreEnabled = {0}")
+  public static Object[] parameters() {
+    return new Object[] {"true", "false"};

Review Comment:
   Do we need to parameterize? I suppose it's fine because there's very few tests in the class, and they're all pretty lightweight so the runtime of the tests won't increase much. I was thinking we can just add a separate test where object store is set to true



-- 
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: issues-unsubscribe@iceberg.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org