delete files in advance #29

kekekenta · 2020-03-03T06:34:02Z

Delete path prefix matched files in GCS bucket first if needed.

sakama · 2020-03-03T06:41:48Z

Why do you want to delete files in advance? Could you provide the background?
In my understanding, the file will be overridden if the same file path already exists there.
Doesn't that work?

kekekenta · 2020-03-03T06:51:46Z

For example, when embulk task size is changed, the number of files will be changed, so unnecessary files will be remained.

$ embulk run -X min_output_tasks=4 -I lib/ example.yml
# this creates 4 files like below in my enviroment.
# 
# test.000.01.csv, test.001.01.csv, test.002.01.csv, test.003.01.csv

$ embulk run -X min_output_tasks=2 -I lib/ example.yml
# this creates 2 files like below in my environment and these files are overwritten, but 002.01.csv, 003.01.csv are remained.
# 
# test.000.01.csv, test.001.01.csv

I would like to assure only transferred files are present.

kekekenta · 2020-03-03T07:00:41Z

In my understanding, the number of output files is varied by input record size, and so on.

sakama

Thanks. I understand your intention and sounds nice.
I added several comments.
Could you also add unit tests if possible?

You don't have to mind CI failure. It is caused by encryption key settings on Travis CI.

sakama · 2020-03-03T07:38:07Z

src/main/java/org/embulk/output/GcsOutputPlugin.java

+        logger.info("delete files");
+        Storage client = createClient(task);
+        try {
+            Objects listResult = client.objects().list(task.getBucket()).setDelimiter("/").setPrefix(task.getPathPrefix()).execute();


Could you use retry with exponential backoff when plugin sends request to Google API?
https://github.com/embulk/embulk-output-gcs/blob/v0.4.4/src/main/java/org/embulk/output/GcsTransactionalFileOutput.java#L167

ok, thanks.

sakama · 2020-03-03T07:39:26Z

src/main/java/org/embulk/output/GcsOutputPlugin.java

+        try {
+            Objects listResult = client.objects().list(task.getBucket()).setDelimiter("/").setPrefix(task.getPathPrefix()).execute();
+            if (listResult.getItems() == null) {
+                logger.info("no files found");


minor: "no files were found" or "no file was found"

sakama · 2020-03-03T07:44:05Z

src/main/java/org/embulk/output/GcsOutputPlugin.java

+            }
+            for (StorageObject item : listResult.getItems()) {
+                logger.info("delete file: {}/{}", item.getBucket(), item.getName());
+                client.objects().delete(item.getBucket(), item.getName()).execute();


Better to move logger.info("delete file: {}/{}", item.getBucket(), item.getName()); after sending requests.
Only logging message will be shown in case delete operation fails,

Could you also use retry with exponential backoff?

sakama · 2020-03-03T07:46:47Z

src/main/java/org/embulk/output/GcsOutputPlugin.java

+            }
+        }
+        catch (ConfigException | IOException ex) {
+            throw Throwables.propagate(ex);


I don't think ConfigException is thrown from any part of this method.
But it would be nice to throw ConfigException if IOException happens.

sakama · 2020-03-03T07:47:25Z

src/main/java/org/embulk/output/PluginTask.java

+
+    @Config("delete_in_advance")
+    @ConfigDefault("false")
+    Boolean getDeleteInAdvance();


sakama · 2020-03-03T07:48:27Z

src/main/java/org/embulk/output/GcsOutputPlugin.java

@@ -78,6 +88,26 @@ public TransactionalFileOutput open(TaskSource taskSource, final int taskIndex)
        return new GcsTransactionalFileOutput(task, client, taskIndex);
    }

+    private void deleteFiles(PluginTask task)
+    {
+        logger.info("delete files");


This message isn't clear to me what this plugin is doing.
"Start delete files operation" or something might be better?

thanks, I changed this to your suggestions.

sakama · 2020-03-03T07:54:40Z

src/main/java/org/embulk/output/GcsOutputPlugin.java

+        Storage client = createClient(task);
+        try {
+            Objects listResult = client.objects().list(task.getBucket()).setDelimiter("/").setPrefix(task.getPathPrefix()).execute();
+            if (listResult.getItems() == null) {


if (listResult.getItems().size() == 0)

listResult.getItems() returns null when no files are found.

kekekenta · 2020-03-03T08:20:47Z

@sakama Thanks a lot. I will check your review comments.

kekekenta · 2020-03-06T06:55:37Z

@sakama I added some test codes and fixed as you pointed out.
Check my code again, please.

kekekenta · 2020-03-14T03:45:23Z

@sakama Sorry for bother you, would you review my code again in your free time!

sakama · 2020-03-16T05:09:40Z

Sorry to have kept you waiting. I'll review in few days.

dmikurube · 2022-02-19T13:31:08Z

@kekekenta Sorry for leaving this pull-request for a long time.

The CI settings of this plugin was not working well (especially with Travis-CI) for these months. We've gotten the CI back with GitHub Actions and a new GCS bucket configuration. It should be working now.

If you still have an intention to work on it to merge, could you rebase it to the latest master, and then (force-)push this, or push as another pull-request? We think about releasing it as v0.6.1 after we release v0.6.0 with the current master.

We had a lot of changes in v0.5.0 from v0.4.4, and going to have more echanges in v0.6.0 soon. For example, catching up with the Embulk v0.10, relocating Java packages, changing its license to the Apache License 2.0, and else. Rebasing wouldn't be very trivial.

dmikurube · 2022-02-19T14:00:27Z

See https://dev.embulk.org/topics/get-ready-for-v0.11-and-v1.0.html (Ja: https://zenn.dev/dmikurube/articles/get-ready-for-embulk-v0-11-and-v1-0) for the Embulk v0.10 catch-up.

chikamura · 2024-10-29T06:47:32Z

@dmikurube @sakama

We apologise for the very late response. 🙇
I have modified it to match the current master branch.
Could you please review this PR when you are available?

sakama reviewed Mar 3, 2020

View reviewed changes

kekekenta force-pushed the delete-in-advance branch 4 times, most recently from 71ed3ba to 1b7e0a9 Compare March 6, 2020 06:53

kekekenta requested a review from sakama March 6, 2020 06:53

dmikurube added this to the v0.6.1 milestone Feb 19, 2022

dmikurube self-requested a review February 19, 2022 13:31

delete files in advance

fb782af

chikamura force-pushed the delete-in-advance branch from 1b7e0a9 to fb782af Compare October 29, 2024 06:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

delete files in advance #29

delete files in advance #29

kekekenta commented Mar 3, 2020

sakama commented Mar 3, 2020

kekekenta commented Mar 3, 2020 •

edited

Loading

kekekenta commented Mar 3, 2020

sakama left a comment •

edited

Loading

sakama Mar 3, 2020

kekekenta Mar 3, 2020

sakama Mar 3, 2020

kekekenta Mar 3, 2020

sakama Mar 3, 2020

sakama Mar 3, 2020

sakama Mar 3, 2020

sakama Mar 3, 2020

kekekenta Mar 3, 2020

sakama Mar 3, 2020

kekekenta Mar 6, 2020

sakama Mar 3, 2020

kekekenta Mar 6, 2020

kekekenta commented Mar 3, 2020

kekekenta commented Mar 6, 2020

kekekenta commented Mar 14, 2020

sakama commented Mar 16, 2020

dmikurube commented Feb 19, 2022 •

edited

Loading

dmikurube commented Feb 19, 2022

chikamura commented Oct 29, 2024 •

edited

Loading

delete files in advance #29

Are you sure you want to change the base?

delete files in advance #29

Conversation

kekekenta commented Mar 3, 2020

sakama commented Mar 3, 2020

kekekenta commented Mar 3, 2020 • edited Loading

kekekenta commented Mar 3, 2020

sakama left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kekekenta commented Mar 3, 2020

kekekenta commented Mar 6, 2020

kekekenta commented Mar 14, 2020

sakama commented Mar 16, 2020

dmikurube commented Feb 19, 2022 • edited Loading

dmikurube commented Feb 19, 2022

chikamura commented Oct 29, 2024 • edited Loading

kekekenta commented Mar 3, 2020 •

edited

Loading

sakama left a comment •

edited

Loading

dmikurube commented Feb 19, 2022 •

edited

Loading

chikamura commented Oct 29, 2024 •

edited

Loading