Initial commit for gcsync command line tool #796

tonyxiaowei · 2025-03-17T19:52:32Z

Implementation of gcsync command

Fix of an issue in the core rsync algorithm

Replace the use of slice()

#. Use full 256 bits checksum

Unit test for gcsyncClient

misolt · 2025-03-18T11:15:01Z

dbsync/gcsync/src/main/java/com/google/edwmigration/dbsync/gcsync/Constants.java

+
+  public static final String INSTRUCTION_FILE_SUFFIX = "instruction";
+
+  public static final String TMP_FILE_SUFFIX = "updated";


I'd consider adding an enum instead of the *_FILE_SUFFIX constants.

misolt · 2025-03-18T11:17:25Z

dbsync/gcsync/src/main/java/com/google/edwmigration/dbsync/gcsync/Constants.java

+
+  public static final String FILES_TO_RSYNC_FILE_NAME = "filesToRsync.txt";
+
+  public static final String JAR_FILE_NAME = "gcsync-all.jar";


For this and other constants which are only used in one file, I'd move it to that file.

misolt · 2025-03-18T11:20:10Z

dbsync/gcsync/src/test/java/com/google/edwmigration/dbsync/gcsync/GcsyncClientTest.java

+  private GcsyncClient clientUnderTest;
+
+  // For test simplicity, define a smaller threshold for "rsync" logic
+  private static final long RSYNC_SIZE_THRESHOLD = 1024;  // e.g. 1KB


Maybe the size threshold could be changed to be a parameter of GsyncClient instead of a constant?

This way we can initialize the test version with a different value rather than using different versions of the constant.

misolt · 2025-03-18T11:24:05Z

dbsync/common/src/main/java/com/google/edwmigration/dbsync/common/InstructionReceiver.java

-      copyStart = -1;
-      copyLength = 0;
-    }
+    this.sourceStream = in.openStream();


Would it work to move this openStream call outside the constructor?

misolt · 2025-03-18T11:40:07Z

dbsync/common/src/main/java/com/google/edwmigration/dbsync/common/InstructionReceiver.java

+  }
+
+  private void flushCopy() throws IOException {
+    if (copyStart == -1) {


What does the value -1 mean for copyStart?

It means copyStart is unset

Please document this, e.g.

... // the start index for copy() if >= 0, or -1 if it is unset @CheckForSigned private int copyStart = -1 ...

shevek-google

Publishing comments.

shevek-google · 2025-03-17T21:03:03Z

dbsync/common/src/main/java/com/google/edwmigration/dbsync/common/InstructionGenerator.java

@@ -76,7 +76,7 @@ public void generate(Consumer<? super Instruction> out, ByteSource in,
          if (cc != null) {
            HashCode strongHashCode = rollingChecksum.getStrongHashCode();
            for (Checksum c : cc) {
-              if (c.getStrongChecksum() == strongHashCode.asLong()) {
+              if (c.getStrongChecksum().equals(strongHashCode.toString())) {


Compute toString() once outside the loop.
Or compare the asBytes() values using Arrays.equals()?

shevek-google · 2025-03-17T21:04:52Z

dbsync/common/src/main/java/com/google/edwmigration/dbsync/common/InstructionReceiver.java

+    copyLength = 0;
+  }
+
+  private void copy(long copyLength) throws IOException {


This, I think, is ByteStreams.copy()

Yes, instead of setting limit here, I find it easier to just implement it which is pretty straightforward.

shevek-google · 2025-03-17T21:07:50Z

dbsync/common/src/main/java/com/google/edwmigration/dbsync/common/InstructionReceiver.java

    }
  }
-}
+
+  private void skip(long length) throws IOException {


Assumption here: The fastest way to skip is to read/skip bytes or call skip().

Probably yes in the case of GCS. Guava's implementation of SlicedByteSource use skip() too.

dbsync/common/src/main/java/com/google/edwmigration/dbsync/common/InstructionReceiver.java

shevek-google · 2025-03-17T21:17:41Z

dbsync/common/src/main/proto/dbsync.proto

@@ -13,7 +13,7 @@ message Checksum {
 	uint64 blockOffset = 1;
 	uint32 blockLength = 2;
 	int32 weakChecksum = 3;
-	int64 strongChecksum = 4;
+	string strongChecksum = 4;


This might be safer as bytes - although bytes are really expensive in protobuf, so it might as well be string.

I think we can leave it as string.

shevek-google · 2025-03-17T21:22:31Z

dbsync/gcsync/src/main/java/com/google/edwmigration/dbsync/gcsync/GcsyncClient.java

+        writer.write(file.getFileName().toString());
+        writer.newLine();
+      }
+    }


Files.writeLines()
CharSink.writeLines is probably easier.

Justification for attempting to avoid a temp file is every so often some customer has java.io.tmpdir misconfigured and this breaks.

shevek-google · 2025-03-17T21:25:00Z

dbsync/gcsync/src/main/java/com/google/edwmigration/dbsync/gcsync/GcsyncClient.java

+    for (Path file : filesToRsync) {
+      Path tmpCheckSumFile = getTemporaryCheckSumFileName(file);
+      try (OutputStream instructionSink = gcsStorage.newByteSink(
+              new URI(tmpBucket).resolve(Util.getInstructionFileName(file.getFileName().toString())))


Why is getTemporaryChecksumFileName here, but getInstructionFileName in util?

shevek-google · 2025-03-17T21:26:10Z

dbsync/gcsync/src/main/java/com/google/edwmigration/dbsync/gcsync/GcsyncClient.java

+              instruction.writeDelimitedTo(instructionSink);
+            } catch (IOException e) {
+              throw new RuntimeException(e);
+            }


I would be tempted to modify generate() to permit the throwing of IOException, and use a custom class instead of Consumer.

interface InstructionConsumer<X extends Exception> { public void accept(Instruction i) throws X; }

shevek-google · 2025-03-17T21:30:04Z

dbsync/gcsync/src/main/java/com/google/edwmigration/dbsync/gcsync/GenerateCheckSumMain.java

+            checksum.writeDelimitedTo(bufferedOutputStream);
+          } catch (IOException e) {
+            throw new RuntimeException("Failed to generate checksum", e);
+          }


Again the typed-exception-in-interface trick.

tonyxiaowei added 4 commits March 17, 2025 17:43

Initial push for gcsync command line tool

331b5c8

Merge branch 'google:main' into main

8b2cfa7

Remove unintentional changes

950a0c2

Some updates

bb67a0e

tonyxiaowei requested review from vladislav-sidorovich and shevek-google as code owners March 17, 2025 19:52

misolt reviewed Mar 18, 2025

View reviewed changes

shevek-google requested changes Mar 18, 2025

View reviewed changes

tonyxiaowei added 2 commits March 19, 2025 05:18

Address pr comments

0e60696

Reimplement gcsbytesource with a cached inputstream

e700ec9

tonyxiaowei force-pushed the main branch from 2b9bf6b to e700ec9 Compare March 20, 2025 02:11

tonyxiaowei added 4 commits March 19, 2025 19:11

Merge branch 'google:main' into main

9639321

remove unnecessary lines in build.gradle

7c064d8

fix a typo

03687be

Remember to close the outputstream

604fcad

AchuthaSourabhC mentioned this pull request Mar 21, 2025

[dbsync] Fix open issues in rsync library and make it functional #782

Closed

tonyxiaowei requested a review from shevek-google March 21, 2025 18:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial commit for gcsync command line tool #796

Initial commit for gcsync command line tool #796

tonyxiaowei commented Mar 17, 2025

misolt Mar 18, 2025 •

edited

Loading

misolt Mar 18, 2025

misolt Mar 18, 2025

misolt Mar 18, 2025

misolt Mar 18, 2025

tonyxiaowei Mar 19, 2025

misolt Mar 19, 2025

shevek-google left a comment

shevek-google Mar 17, 2025

tonyxiaowei Mar 19, 2025

shevek-google Mar 17, 2025

tonyxiaowei Mar 19, 2025

shevek-google Mar 17, 2025

tonyxiaowei Mar 19, 2025

shevek-google Mar 17, 2025

tonyxiaowei Mar 19, 2025

shevek-google Mar 17, 2025

tonyxiaowei Mar 19, 2025

shevek-google Mar 17, 2025

tonyxiaowei Mar 19, 2025

shevek-google Mar 17, 2025

tonyxiaowei Mar 19, 2025

shevek-google Mar 17, 2025

tonyxiaowei Mar 19, 2025


		public static final String INSTRUCTION_FILE_SUFFIX = "instruction";

		public static final String TMP_FILE_SUFFIX = "updated";


		public static final String FILES_TO_RSYNC_FILE_NAME = "filesToRsync.txt";

		public static final String JAR_FILE_NAME = "gcsync-all.jar";

Initial commit for gcsync command line tool #796

Are you sure you want to change the base?

Initial commit for gcsync command line tool #796

Conversation

tonyxiaowei commented Mar 17, 2025

Implementation of gcsync command

Fix of an issue in the core rsync algorithm

Replace the use of slice()

Unit test for gcsyncClient

misolt Mar 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shevek-google left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misolt Mar 18, 2025 •

edited

Loading