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

AchuthaSourabhC · 2025-03-08T07:17:39Z

This PR fixes all the open issues to get the library functional

Issues being fixed

First sync failed when file is not present in target, we have to create and directly copy the file first time
Exception was thrown if file size was less than min file size constant, in this case file can just be copied directly
Staging files (checksum, instruction,staged) were not stored in proper directory structure and would cause problems when syncing multiple files
once rsycn was done and staged file is created based on instructions it was note being moved/copied to target and target file was left out of data
Staging files were not getting cleaned up at the end
If trailing slash was not present in staging path invalid staging file paths were getting created. ( need to add if missing)
Cloud run job were not deleted at the end

Other changes

Added a param to decide if staging files should be deleted (its helpful to not delete for debugging and testing)
Fixed formatting and naming issues ( lot of the updates are related to this)

Testing:
test manually with following params

--project=achuthasourabh-test --location=us-west1 --target_file=gs://rsync_target/data/test.csv --source_file=file:///usr/local/google/home/achuthasourabh/Downloads/annual-enterprise-survey-2023-financial-year-provisional-size-bands.csv --staging_bucket=gs://rsync_staging --delete_staging_files=false

Screenshot of staging bucket files

Screenshot of target file

Note: Need to setup proper integ/e2e tests but for now trying to get it working and unblock dependent work

tonyxiaowei · 2025-03-14T22:46:24Z

dbsync/client/src/main/java/com/google/edwmigration/dbsync/client/RsyncClient.java

+            try {
+              instruction.writeDelimitedTo(instructionStream);
+            } catch (IOException e) {
+              throw new RuntimeException("Failed to write instructions", e);


Will still delete the intermediate files like checksum or instructions if it throws here?

I was thinking whats best way to handle this because failure can happen at different stages and may be some of the files we are deleting are not even created.
One option is we do cleanup on best effort in a finally block at top level that encompasses all exceptions. But ignore any error while trying to delete including file not found errors.

tonyxiaowei · 2025-03-14T22:47:30Z

dbsync/client/src/main/java/com/google/edwmigration/dbsync/client/RsyncClient.java

    }

    // Reconstruct on GCS
    server.reconstruct();

-    //TODO delete the jobs from cloudrun
+    server.deleteJob(Mode.GENERATE);


Is the goal to have these running as servers instead of as a one time command line program later?

To my knowledge we wanted to run it as jobs running on demand, since frequency of execution might actually be less.
Though i was checking if there was a way to re-use deployed jobs and run with different inputs instead of deploying new jobs for each file. But for now couldn't find good solution for this.

tonyxiaowei · 2025-03-14T22:51:01Z

dbsync/server/src/main/java/com/google/edwmigration/dbsync/server/CloudRunServerAPI.java

-        project, location, Mode.GENERATE,
-        String.format("%s && %s", jarDownloadCommand, generateCommand)
-    );
+        project,


I'd suggest separating these formatting changes next time. It makes it harder to see what are real changes and what are just style changes.

Yes will keep in mind. I initially had setup "format on save" and didn't realize would end up with big diff

tonyxiaowei · 2025-03-14T22:55:01Z

dbsync/server/src/main/java/com/google/edwmigration/dbsync/server/GCSTarget.java

+    //  If target files path is gs://target_bucket/dir/target/file.txt
+    //  Staging bucket is gs://target_bucket/
+    //  Then staging files will be stored as gs://target_bucket/dir/target/file.txt/staged
+    String targetRelativePath = UriUtil.getRelativePath(targetUri);


This variable is used only once a line below, you can inline it

Yes will update

tonyxiaowei · 2025-03-14T23:01:36Z

dbsync/server/src/main/java/com/google/edwmigration/dbsync/server/RsyncServer.java

+              // TODO: Switch to simple write to save storage and read by byte size
+              checksum.writeDelimitedTo(checksumStream);
+            } catch (IOException e) {
+              throw new RuntimeException("Failed to generate checksum", e);


We won't delete the staging files in this case?

Similar to other comment #782 (comment)
Need a good way cleanup regardless of the error

tonyxiaowei · 2025-03-14T23:02:45Z

dbsync/server/src/main/java/com/google/edwmigration/dbsync/server/GCSTarget.java

@@ -55,4 +63,14 @@ public ByteSource getInstructionsByteSource() {
  public ByteSink getInstructionsByteSink() {
    return storage.newByteSink(instructionUri);
  }
+
+  @Override
+  public void moveStagedFileToTarget(boolean deleteStagingFiles) {


I think it's better to have a deleteStagedFiles method instead of passing in boolean flag here.

Ya make sense. Will update

[b/400338195] Fix how staging files are stored and target is updated

d39671f

AchuthaSourabhC force-pushed the update-rsync-staging-logic branch from 156aeb9 to d39671f Compare March 8, 2025 15:26

AchuthaSourabhC added 2 commits March 8, 2025 16:10

[b/400969319] Upload file directly if is not found or small

8f60431

[b/400689198] Delete cloud run jobs at the end

8244c8e

AchuthaSourabhC changed the title ~~[b/400338195] Fix how staging files are stored and target is updated~~ [dbsync] Fix how staging files are stored and target is updated Mar 9, 2025

AchuthaSourabhC changed the title ~~[dbsync] Fix how staging files are stored and target is updated~~ [dbsync] Fix open issues in rsync library and make it functional e2e Mar 9, 2025

Update formatting and comments

a5f7bfe

AchuthaSourabhC changed the title ~~[dbsync] Fix open issues in rsync library and make it functional e2e~~ [dbsync] Fix open issues in rsync library and make it functional Mar 9, 2025

AchuthaSourabhC marked this pull request as ready for review March 9, 2025 11:31

AchuthaSourabhC requested review from vladislav-sidorovich and shevek-google as code owners March 9, 2025 11:31

tonyxiaowei approved these changes Mar 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

AchuthaSourabhC commented Mar 8, 2025 •

edited

Loading

tonyxiaowei Mar 14, 2025

AchuthaSourabhC Mar 17, 2025

tonyxiaowei Mar 14, 2025

AchuthaSourabhC Mar 17, 2025

tonyxiaowei Mar 14, 2025

AchuthaSourabhC Mar 17, 2025

tonyxiaowei Mar 14, 2025

AchuthaSourabhC Mar 17, 2025

tonyxiaowei Mar 14, 2025

AchuthaSourabhC Mar 17, 2025

tonyxiaowei Mar 14, 2025

AchuthaSourabhC Mar 17, 2025

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

Are you sure you want to change the base?

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

Conversation

AchuthaSourabhC commented Mar 8, 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

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

AchuthaSourabhC commented Mar 8, 2025 •

edited

Loading