Add java storage implementation for delete objects method #1079

shilpi23pandey · 2023-12-04T18:51:53Z

Storage gRPC API doesnt support batch requests so BatchExecutor emulates batching at the connector client to have parity with the HTTP implementation.

arunkumarchacko · 2023-12-05T08:15:11Z

/gcbrun

arunkumarchacko · 2023-12-06T06:23:23Z

/gcbrun

arunkumarchacko · 2023-12-06T11:12:16Z

gcsio/src/main/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageClientImpl.java

+    for (StorageResourceId object : fullObjectNames) {
+      queueSingleObjectDelete(object, innerExceptions, executor, 0);
+    }
+    executor.shutdown();


please do this in finally block

arunkumarchacko · 2023-12-06T11:16:45Z

gcsio/src/main/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageClientImpl.java

+            public void onFailure(Throwable throwable) {
+              innerExceptions.add(
+                  new IOException(
+                      String.format("Error deleting %s, stage 1", resourceId), throwable));


Stage1 does not make sense. Please make the explicit and say something like "getting object".

This is copied from the existing implementation.

hadoop-connectors/gcsio/src/main/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageImpl.java

Line 851 in 48891bf

String.format("Error deleting '%s', stage 1", resourceId), cause));

. Keeping this as is for the sake of parity.

arunkumarchacko · 2023-12-06T11:24:41Z

gcsio/src/main/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageClientImpl.java

+      // version of the object.
+      grpcManualBatchExecutor.queue(
+          () ->
+              storage.get(


On which thread we will be getting the callback? Is there any advantage of making this async. i.e. why not make this a blocking call?

The callback will be called on the same thread. The callable(get request here) will be marked as complete only after the callback is executed https://github.com/GoogleCloudDataproc/hadoop-connectors/pull/1079/files#diff-c13e75f742b44b8dfd6bcfe5b7014bc938cf8fd87790f3359e7afbca76bfad21R76. The callback queues a delete request which can then be async.

arunkumarchacko · 2023-12-06T11:26:00Z

gcsio/src/main/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageClientImpl.java

+          queueSingleObjectDelete(
+              resourceId, innerExceptions, grpcManualBatchExecutor, attempt + 1);
+        } else {
+          innerExceptions.add(


We usually expect the inner exceptions order to match the operations order. Is it guaranteed here?

The inner exceptions will match the operations order.

gcsio/src/main/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageClientImpl.java

arunkumarchacko · 2023-12-06T11:42:53Z

gcsio/src/main/java/com/google/cloud/hadoop/gcsio/GrpcManualBatchExecutor.java

+ *
+ * <p>Instance of this class can not be used again after {@link #shutdown()} method has been called.
+ */
+public class GrpcManualBatchExecutor {


Let us not make this public

arunkumarchacko · 2023-12-06T11:49:14Z

gcsio/src/main/java/com/google/cloud/hadoop/gcsio/GrpcManualBatchExecutor.java

+
+  private final ExecutorService requestsExecutor;
+
+  private final Queue<Future<Void>> responseFutures = new ConcurrentLinkedQueue<>();


Any reason why normal queue is not sufficient here?

This is to ensure thread safety, this collection will be shared across threads. We do have cases where a task running on a thread will try to queue another task(get object metadata will queue delete on success) so there may be cases where two threads will try to modify this collection.

arunkumarchacko · 2023-12-06T11:54:37Z

gcsio/src/main/java/com/google/cloud/hadoop/gcsio/GrpcManualBatchExecutor.java

+  /** Awaits until all sent requests are completed. Should be serialized */
+  private void awaitRequestsCompletion() throws IOException {
+    while (!responseFutures.isEmpty()) {
+      getFromFuture(responseFutures.remove());


do we have to have catch block here? i.e. can getFromFuture throw exception?

getFromFuture

hadoop-connectors/gcsio/src/main/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageFileSystemImpl.java

Line 736 in 48891bf

static <T> T getFromFuture(Future<T> future) throws IOException {

handles the exception and wraps them in the IOException.

arunkumarchacko · 2023-12-06T11:59:16Z

...t/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageFileSystemNewIntegrationTestBase.java

@@ -60,6 +60,8 @@ public abstract class GoogleCloudStorageFileSystemNewIntegrationTestBase {

  protected GoogleCloudStorageFileSystem gcsFs;

+  protected static boolean isTracingSupported = true;


please add a comment explaining why we needed this.

arunkumarchacko · 2023-12-06T12:00:40Z

gcsio/src/main/java/com/google/cloud/hadoop/gcsio/GrpcManualBatchExecutor.java

+ *
+ * <p>Instance of this class can not be used again after {@link #shutdown()} method has been called.
+ */
+public class GrpcManualBatchExecutor {


Let us give this a more generic name. Let us get rid of "Grpc" and "Manual"

arunkumarchacko · 2023-12-07T10:37:29Z

/gcbrun

shilpi23pandey · 2023-12-11T08:55:26Z

/gcbrun

shilpi23pandey marked this pull request as draft December 4, 2023 18:52

shilpi23pandey changed the title ~~Move to java storage objects~~ Dec 4, 2023

shilpi23pandey force-pushed the move-to-java-storage-objects branch from 4eea78c to d2b9944 Compare December 5, 2023 07:56

shilpi23pandey force-pushed the move-to-java-storage-objects branch 3 times, most recently from ec960d2 to 4f4a0d2 Compare December 6, 2023 06:14

shilpi23pandey marked this pull request as ready for review December 6, 2023 07:12

arunkumarchacko reviewed Dec 6, 2023

View reviewed changes

shilpi23pandey force-pushed the move-to-java-storage-objects branch from 4f4a0d2 to 9f427c3 Compare December 7, 2023 09:51

shilpi23pandey requested a review from arunkumarchacko December 8, 2023 07:02

arunkumarchacko approved these changes Dec 11, 2023

View reviewed changes

shilpi23pandey added 3 commits December 11, 2023 14:17

Add java-storage implementation for delete objects API.

d365f83

Add java-storage implementation for delete objects API.

90fea6b

Use a direct executor for 0 thread count configuration.

5188dac

shilpi23pandey force-pushed the move-to-java-storage-objects branch from 9f427c3 to 5188dac Compare December 11, 2023 08:54

shilpi23pandey merged commit 84652b2 into GoogleCloudDataproc:master Dec 11, 2023
4 checks passed

shilpi23pandey deleted the move-to-java-storage-objects branch December 11, 2023 09:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add java storage implementation for delete objects method #1079

Add java storage implementation for delete objects method #1079

shilpi23pandey commented Dec 4, 2023 •

edited

Loading

arunkumarchacko commented Dec 5, 2023

arunkumarchacko commented Dec 6, 2023

arunkumarchacko Dec 6, 2023

shilpi23pandey Dec 7, 2023

arunkumarchacko Dec 6, 2023

shilpi23pandey Dec 7, 2023

arunkumarchacko Dec 6, 2023

shilpi23pandey Dec 7, 2023

arunkumarchacko Dec 6, 2023

shilpi23pandey Dec 7, 2023

arunkumarchacko Dec 6, 2023

shilpi23pandey Dec 7, 2023

arunkumarchacko Dec 6, 2023

shilpi23pandey Dec 7, 2023

arunkumarchacko Dec 6, 2023

shilpi23pandey Dec 7, 2023

arunkumarchacko Dec 6, 2023

shilpi23pandey Dec 7, 2023

arunkumarchacko Dec 6, 2023

shilpi23pandey Dec 7, 2023

arunkumarchacko commented Dec 7, 2023

shilpi23pandey commented Dec 11, 2023


		private final ExecutorService requestsExecutor;

		private final Queue<Future<Void>> responseFutures = new ConcurrentLinkedQueue<>();

		@@ -60,6 +60,8 @@ public abstract class GoogleCloudStorageFileSystemNewIntegrationTestBase {

		protected GoogleCloudStorageFileSystem gcsFs;

		protected static boolean isTracingSupported = true;

Add java storage implementation for delete objects method #1079

Add java storage implementation for delete objects method #1079

Conversation

shilpi23pandey commented Dec 4, 2023 • edited Loading

arunkumarchacko commented Dec 5, 2023

arunkumarchacko commented Dec 6, 2023

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

Choose a reason for hiding this comment

arunkumarchacko commented Dec 7, 2023

shilpi23pandey commented Dec 11, 2023

shilpi23pandey commented Dec 4, 2023 •

edited

Loading