Adjust to review comments and add unit tests

Author: tkaymak

sdks/java/io/debezium/src/main/java/org/apache/beam/io/debezium/DebeziumTransformRegistrar.java

@@ -152,7 +152,7 @@ public PTransform<PBegin, PCollection<String>> buildExternal(Configuration confi
       if (configuration.offsetStoragePath != null) {
         readTransform =
             readTransform.withOffsetRetainer(
-                new FileSystemOffsetRetainer(configuration.offsetStoragePath));
+                FileSystemOffsetRetainer.of(configuration.offsetStoragePath));
       }
 
       return readTransform;

sdks/java/io/debezium/src/main/java/org/apache/beam/io/debezium/FileSystemOffsetRetainer.java

@@ -26,6 +26,7 @@
 import java.nio.channels.Channels;
 import java.nio.channels.ReadableByteChannel;
 import java.nio.channels.WritableByteChannel;
+import java.util.Collections;
 import java.util.Map;
 import org.apache.beam.sdk.io.FileSystems;
 import org.apache.beam.sdk.io.fs.ResourceId;
@@ -51,7 +52,7 @@
  * DebeziumIO.read()
  *     .withConnectorConfiguration(config)
  *     .withOffsetRetainer(
- *         new FileSystemOffsetRetainer("gs://my-bucket/debezium/orders-offset.json"))
+ *         FileSystemOffsetRetainer.of("gs://my-bucket/debezium/orders-offset.json"))
  *     .withFormatFunction(myMapper);
  * }</pre>
  *
@@ -60,12 +61,13 @@
  * <pre>{@code
  * DebeziumIO.read()
  *     .withConnectorConfiguration(config)
- *     .withOffsetRetainer(new FileSystemOffsetRetainer("/tmp/debezium-offset.json"))
+ *     .withOffsetRetainer(FileSystemOffsetRetainer.of("/tmp/debezium-offset.json"))
  *     .withFormatFunction(myMapper);
  * }</pre>
  *
- * <p><b>Note:</b> writes are not atomic. If the pipeline is killed mid-write, the offset file may
- * be corrupt. In that case, delete the file and the connector will restart from the beginning.
+ * <p><b>Note:</b> writes are performed atomically: the offset is first written to a {@code .tmp}
+ * sibling file and then renamed to the final path, so a mid-write crash leaves the previous offset
+ * intact.
  */
 public class FileSystemOffsetRetainer implements OffsetRetainer {
 
@@ -77,10 +79,18 @@ public class FileSystemOffsetRetainer implements OffsetRetainer {
   // ObjectMapper is thread-safe after configuration and does not need to be serialised.
   private transient @Nullable ObjectMapper objectMapper;
 
-  public FileSystemOffsetRetainer(String path) {
+  // Tracks the last successfully saved offset so repeated identical saves are skipped.
+  private transient @Nullable Map<String, Object> lastSavedOffset;
+
+  private FileSystemOffsetRetainer(String path) {
     this.path = path;
   }
 
+  /** Creates a new {@code FileSystemOffsetRetainer} that stores the offset at {@code path}. */
+  public static FileSystemOffsetRetainer of(String path) {
+    return new FileSystemOffsetRetainer(path);
+  }
+
   private ObjectMapper mapper() {
     if (objectMapper == null) {
       objectMapper = new ObjectMapper();
@@ -89,8 +99,9 @@ private ObjectMapper mapper() {
   }
 
   /**
-   * Reads the offset JSON file and returns its contents, or {@code null} if the file does not exist
-   * or cannot be read.
+   * Reads the offset JSON file and returns its contents, or {@code null} if the file does not yet
+   * exist (first run). Throws {@link RuntimeException} if the file exists but cannot be read, to
+   * prevent silently reprocessing data from the beginning.
    */
   @Override
   public @Nullable Map<String, Object> loadOffset() {
@@ -106,24 +117,43 @@ private ObjectMapper mapper() {
       LOG.info("OffsetRetainer: no offset file found at {}; starting from the beginning.", path);
       return null;
     } catch (IOException e) {
-      LOG.warn(
-          "OffsetRetainer: failed to load offset from {}; starting from the beginning.", path, e);
-      return null;
+      throw new RuntimeException(
+          "OffsetRetainer: failed to read offset from "
+              + path
+              + ". "
+              + "Delete the file to restart from the beginning.",
+          e);
     }
   }
 
   /**
-   * Serialises {@code offset} to JSON and writes it to the configured path, overwriting any
-   * existing file. Errors are logged as warnings and swallowed so the pipeline continues.
+   * Serialises {@code offset} to JSON and writes it atomically to the configured path.
+   *
+   * <p>If the offset is identical to the last successfully written one, the write is skipped to
+   * avoid unnecessary I/O on every checkpoint.
+   *
+   * <p>Otherwise the data is first written to a {@code .tmp} sibling file and then renamed to the
+   * final path, so a mid-write crash leaves the previous offset intact.
+   *
+   * <p>Errors are logged as warnings and swallowed so the pipeline continues.
    */
   @Override
   public void saveOffset(Map<String, Object> offset) {
+    if (offset.equals(lastSavedOffset)) {
+      LOG.debug("OffsetRetainer: offset unchanged, skipping write to {}", path);
+      return;
+    }
+    String tmpPath = path + ".tmp";
     try {
-      ResourceId resourceId = FileSystems.matchNewResource(path, /* isDirectory= */ false);
-      try (WritableByteChannel channel = FileSystems.create(resourceId, "application/json");
+      ResourceId tmpResourceId = FileSystems.matchNewResource(tmpPath, /* isDirectory= */ false);
+      try (WritableByteChannel channel = FileSystems.create(tmpResourceId, "application/json");
           OutputStream stream = Channels.newOutputStream(channel)) {
         mapper().writeValue(stream, offset);
       }
+      ResourceId finalResourceId = FileSystems.matchNewResource(path, /* isDirectory= */ false);
+      FileSystems.rename(
+          Collections.singletonList(tmpResourceId), Collections.singletonList(finalResourceId));
+      lastSavedOffset = offset;
       LOG.debug("OffsetRetainer: saved offset to {}: {}", path, offset);
     } catch (IOException e) {
       LOG.warn(

sdks/java/io/debezium/src/test/java/org/apache/beam/io/debezium/FileSystemOffsetRetainerTest.java

@@ -0,0 +1,131 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.io.debezium;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertThrows;
+import static org.junit.Assert.assertTrue;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Paths;
+import java.util.Map;
+import org.apache.beam.sdk.io.FileSystems;
+import org.apache.beam.sdk.options.PipelineOptionsFactory;
+import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.collect.ImmutableMap;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Unit tests for {@link FileSystemOffsetRetainer}. */
+@RunWith(JUnit4.class)
+public class FileSystemOffsetRetainerTest {
+
+  @Rule public TemporaryFolder tmpFolder = new TemporaryFolder();
+
+  @Before
+  public void setUp() {
+    FileSystems.setDefaultPipelineOptions(PipelineOptionsFactory.create());
+  }
+
+  @Test
+  public void testLoadOffsetReturnNullWhenFileIsMissing() {
+    String path = tmpFolder.getRoot().getAbsolutePath() + "/nonexistent.json";
+    assertNull(FileSystemOffsetRetainer.of(path).loadOffset());
+  }
+
+  @Test
+  public void testLoadOffsetThrowsWhenFileIsUnreadable() throws Exception {
+    String path = tmpFolder.getRoot().getAbsolutePath() + "/offset.json";
+    FileSystemOffsetRetainer retainer = FileSystemOffsetRetainer.of(path);
+    retainer.saveOffset(ImmutableMap.of("lsn", "100"));
+
+    // Corrupt the file so JSON parsing fails.
+    Files.newBufferedWriter(Paths.get(path), StandardCharsets.UTF_8).close(); // truncate to empty
+    assertThrows(RuntimeException.class, retainer::loadOffset);
+  }
+
+  @Test
+  public void testSaveAndLoadOffsetRoundTrip() {
+    String path = tmpFolder.getRoot().getAbsolutePath() + "/offset.json";
+    FileSystemOffsetRetainer retainer = FileSystemOffsetRetainer.of(path);
+
+    retainer.saveOffset(ImmutableMap.of("file", "binlog.000001", "pos", "156"));
+
+    Map<String, Object> loaded = retainer.loadOffset();
+    assertNotNull(loaded);
+    assertEquals("binlog.000001", loaded.get("file"));
+    assertEquals("156", loaded.get("pos"));
+  }
+
+  @Test
+  public void testSaveOffsetSkipsWriteWhenOffsetUnchanged() throws Exception {
+    String path = tmpFolder.getRoot().getAbsolutePath() + "/offset.json";
+    FileSystemOffsetRetainer retainer = FileSystemOffsetRetainer.of(path);
+    Map<String, Object> offset = ImmutableMap.of("lsn", "100");
+
+    retainer.saveOffset(offset);
+    long modifiedAfterFirstSave = new File(path).lastModified();
+
+    // Second call with the same offset should not touch the file.
+    Thread.sleep(10); // ensure mtime would differ if a write occurred
+    retainer.saveOffset(offset);
+    assertEquals(modifiedAfterFirstSave, new File(path).lastModified());
+  }
+
+  @Test
+  public void testSaveOffsetLeavesNoTmpFile() {
+    String path = tmpFolder.getRoot().getAbsolutePath() + "/offset.json";
+    FileSystemOffsetRetainer.of(path).saveOffset(ImmutableMap.of("lsn", "28160840"));
+
+    assertTrue("Final offset file should exist", new File(path).exists());
+    assertFalse("Temp file should not remain after rename", new File(path + ".tmp").exists());
+  }
+
+  @Test
+  public void testSerializedRetainerCanLoadAfterDeserialization() throws Exception {
+    String path = tmpFolder.getRoot().getAbsolutePath() + "/offset.json";
+    FileSystemOffsetRetainer original = FileSystemOffsetRetainer.of(path);
+    original.saveOffset(ImmutableMap.of("lsn", "12345"));
+
+    ByteArrayOutputStream bos = new ByteArrayOutputStream();
+    try (ObjectOutputStream oos = new ObjectOutputStream(bos)) {
+      oos.writeObject(original);
+    }
+    FileSystemOffsetRetainer deserialized;
+    try (ObjectInputStream ois =
+        new ObjectInputStream(new ByteArrayInputStream(bos.toByteArray()))) {
+      deserialized = (FileSystemOffsetRetainer) ois.readObject();
+    }
+
+    Map<String, Object> loaded = deserialized.loadOffset();
+    assertNotNull(loaded);
+    assertEquals("12345", loaded.get("lsn"));
+  }
+}