[GitHub] [incubator-iceberg] rdblue commented on a change in pull request #818: Refactor and improve BaseRewriteManifests

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

rdblue commented on a change in pull request #818: Refactor and improve BaseRewriteManifests
URL: https://github.com/apache/incubator-iceberg/pull/818#discussion_r386673835
 
 

 ##########
 File path: core/src/main/java/org/apache/iceberg/BaseRewriteManifests.java
 ##########
 @@ -325,59 +327,38 @@ long getManifestTargetSizeBytes() {
   }
 
   class WriterWrapper {
-    private final Map<Integer, ManifestWriter> manifestWritersBySpecId = Maps.newConcurrentMap();
+    private final PartitionSpec spec;
+    private ManifestWriter writer;
 
-    synchronized void addEntry(ManifestEntry entry, int partitionSpecId) {
-      getWriter(partitionSpecId).existing(entry);
+    WriterWrapper(PartitionSpec spec) {
+      this.spec = spec;
     }
 
-    synchronized ManifestWriter getWriter(int partitionSpecId) {
-      ManifestWriter writer = manifestWritersBySpecId.get(partitionSpecId);
-      if (writer != null) {
-        if (writer.length() < getManifestTargetSizeBytes()) {
-          return writer;
-        } else {
-          close(partitionSpecId);
-        }
+    synchronized void addEntry(ManifestEntry entry) {
+      if (writer == null) {
+        writer = newWriter();
+      } else if (writer.length() >= getManifestTargetSizeBytes()) {
+        close();
+        writer = newWriter();
       }
+      writer.existing(entry);
+    }
 
-      // create ManifestWriter with the correct partitionSpec
-      PartitionSpec partitionSpec = specsById.get(partitionSpecId);
-      OutputFile outputFile = manifestPath(manifestSuffix.getAndIncrement());
-      writer = new ManifestWriter(partitionSpec, outputFile, snapshotId());
-      manifestWritersBySpecId.put(partitionSpecId, writer);
-      return writer;
+    private ManifestWriter newWriter() {
+      return new ManifestWriter(spec, manifestPath(manifestSuffix.getAndIncrement()), snapshotId());
     }
 
-    synchronized void close(int partitionSpecId) {
-      if (manifestWritersBySpecId != null) {
+    void close() {
 
 Review comment:
   If a method is not safe to be called from multiple threads, then it should use `synchronized`. We don't want to rely on the current set of callers.

----------------------------------------------------------------
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


With regards,
Apache Git Services

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