add support for userProject in java-storage #1205

singhravidutt · 2024-07-04T05:13:08Z

No description provided.

singhravidutt · 2024-07-04T05:14:22Z

/gcbrun

codecov · 2024-07-05T08:38:49Z

Codecov Report

Attention: Patch coverage is 72.00000% with 14 lines in your changes missing coverage. Please review.

Project coverage is 80.87%. Comparing base (2152f44) to head (6c75bb1).
Report is 10 commits behind head on branch-2.2.x.

Files with missing lines	Patch %	Lines
...oogleCloudStorageClientGrpcTracingInterceptor.java	66.66%	2 Missing and 2 partials ⚠️
...oud/hadoop/gcsio/GoogleCloudStorageClientImpl.java	57.14%	3 Missing ⚠️
...oop/gcsio/GoogleCloudStorageClientReadChannel.java	25.00%	2 Missing and 1 partial ⚠️
...oogle/cloud/hadoop/gcsio/RequesterPaysManager.java	88.23%	1 Missing and 1 partial ⚠️
...ogle/cloud/hadoop/util/GrpcErrorTypeExtractor.java	33.33%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##             branch-2.2.x    #1205      +/-   ##
==================================================
+ Coverage           80.82%   80.87%   +0.05%     
- Complexity           2413     2425      +12     
==================================================
  Files                 167      168       +1     
  Lines               10790    10836      +46     
  Branches             1198     1204       +6     
==================================================
+ Hits                 8721     8764      +43     
+ Misses               1541     1536       -5     
- Partials              528      536       +8

Flag	Coverage Δ
hadoop2integrationtest	`64.00% <66.00%> (+0.01%)`	⬆️
hadoop2unittest	`67.05% <42.00%> (-0.13%)`	⬇️
hadoop3integrationtest	`63.85% <66.00%> (+<0.01%)`	⬆️
hadoop3unittest	`67.11% <42.00%> (-0.13%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

arunkumarchacko · 2024-07-08T03:39:14Z

.../main/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageClientGrpcTracingInterceptor.java

@@ -43,10 +44,14 @@
 public class GoogleCloudStorageClientGrpcTracingInterceptor implements ClientInterceptor {
  private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
  public static final String IDEMPOTENCY_TOKEN_HEADER = "x-goog-gcs-idempotency-token";
+  public static final String USER_PROJECT_HEADER = "x-goog-user-project";


Why are we doing this as an interceptor? Why not veneer provide this as a config while creating the client. Even better, they should be able to figure this out from the GCE VM they are running (if it is running from VM), correct?

Why are we doing this as an interceptor?

This is Trace interceptor, here we trace the request for userProject filed. If this field is in headers we do log it. It points towards the requesterPays filed being set for gcsbucket in request.

Why not veneer provide this as a config while creating the client.

Via this code we are not updating the header to use the provided project but other way round. Java-storage client do provide a way for requester-pays to be set.

No. It's a userProject header is the one which carries information of requesterpays feature of gcs bucket. This has nothing to with the account/project the VM is in. This feature can even be used from machines which are not part of GCP.

arunkumarchacko · 2024-07-08T03:41:26Z

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

@@ -114,6 +116,8 @@ public class GoogleCloudStorageClientImpl extends ForwardingGoogleCloudStorage {
            ? createStorage(
                credentials, options, gRPCInterceptors, pCUExecutorService, downscopedAccessTokenFn)
            : clientLibraryStorage;
+    this.requesterPaysManager =


Looks like we are adding more than the userProject in this PR? If yes, please update the PR descriptions.

No. it is very specific to requesterPays feature. RequesterPaysManager is created with an idea that multiple GoogleCloundStorage client will be able to share the same caching logic of requesterPaysInfo.

arunkumarchacko · 2024-07-15T11:14:22Z

util/src/main/java/com/google/cloud/hadoop/util/GrpcErrorTypeExtractor.java

      default:
        return ErrorType.UNKNOWN;
    }
  }
+
+  @Override
+  public boolean userProjectMissing(Exception error) {


rename to isUserProjectMissingError?

arunkumarchacko · 2024-07-15T11:20:21Z

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

+  }
+
+  public boolean requesterShouldPay(String bucketName) {
+    if (bucketName == null) {


when can this happen?

arunkumarchacko · 2024-07-15T11:26:17Z

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

+import java.util.function.Function;
+
+@VisibleForTesting
+public class RequesterPaysManager {


Why not using this in the JSON path as well? Looks like now autobuckets will be there in both GCSImpl and GCSClientImpl?

arunkumarchacko · 2024-07-15T11:39:24Z

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

@@ -270,6 +279,15 @@ private Storage createStorage(
        .getService();
  }

+  private Boolean shouldRequesterPay(String bucketName) {


why not move this to RequestPaysManager?

arunkumarchacko · 2024-07-15T11:45:32Z

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

@@ -582,6 +586,10 @@ private BlobSourceOption[] generateReadOptions(BlobId blobId) {
        blobReadOptions.add(
            BlobSourceOption.decryptionKey(storageOptions.getEncryptionKey().value()));
      }
+      if (requesterShouldPay.apply((blobId.getBucket()))) {


Why can't we have this as a boolean value per GCSClientReadChannel? i.e. what is the value of recomputing this for reach GCS ReadChannel?

arunkumarchacko · 2024-07-15T11:46:38Z

gcsio/src/test/java/com/google/cloud/hadoop/gcsio/RequesterPaysIntegrationTest.java

+  public void before() throws IOException {
+    bucketHelper = new TestBucketHelper("dataproc-requesterpays");
+    testBucket = bucketHelper.getUniqueBucketPrefix();
+    // Initalizing a storage client as gcsio abstraction don't offer a way to ingested


arunkumarchacko · 2024-07-15T11:47:04Z

gcsio/src/test/java/com/google/cloud/hadoop/gcsio/RequesterPaysIntegrationTest.java

+    bucketHelper = new TestBucketHelper("dataproc-requesterpays");
+    testBucket = bucketHelper.getUniqueBucketPrefix();
+    // Initalizing a storage client as gcsio abstraction don't offer a way to ingested
+    // "requesterPays" filed while creating bucket.


arunkumarchacko · 2024-07-15T11:50:16Z

gcsio/src/test/java/com/google/cloud/hadoop/gcsio/RequesterPaysIntegrationTest.java

+  }
+
+  @Test
+  public void requesterPays_autoMode() throws Exception {


Do we need testing no-auto use cases?

arunkumarchacko · 2024-07-15T11:51:12Z

gcsio/src/test/java/com/google/cloud/hadoop/gcsio/RequesterPaysManagerTest.java

+            .setMode(RequesterPaysMode.CUSTOM)
+            .setBuckets(requesterPaysBuckets)
+            .build();
+    RequesterPaysManager manager = new RequesterPaysManager(options, this::shouldRequesterPays);


consider moving this to "before test" method

singhravidutt and others added 2 commits July 4, 2024 10:41

add support for userProject in java-storage

281da70

Merge branch 'branch-2.2.x' into requesterPaysgRPC

6c75bb1

singhravidutt marked this pull request as ready for review July 5, 2024 10:05

singhravidutt requested a review from arunkumarchacko July 5, 2024 10:07

arunkumarchacko reviewed Jul 8, 2024

View reviewed changes

arunkumarchacko reviewed Jul 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for userProject in java-storage #1205

add support for userProject in java-storage #1205

singhravidutt commented Jul 4, 2024

singhravidutt commented Jul 4, 2024

codecov bot commented Jul 5, 2024 •

edited

Loading

arunkumarchacko Jul 8, 2024

singhravidutt Jul 8, 2024

arunkumarchacko Jul 8, 2024

singhravidutt Jul 8, 2024

arunkumarchacko Jul 15, 2024

arunkumarchacko Jul 15, 2024

arunkumarchacko Jul 15, 2024

arunkumarchacko Jul 15, 2024

arunkumarchacko Jul 15, 2024

arunkumarchacko Jul 15, 2024

arunkumarchacko Jul 15, 2024

arunkumarchacko Jul 15, 2024

arunkumarchacko Jul 15, 2024

add support for userProject in java-storage #1205

Are you sure you want to change the base?

add support for userProject in java-storage #1205

Conversation

singhravidutt commented Jul 4, 2024

singhravidutt commented Jul 4, 2024

codecov bot commented Jul 5, 2024 • edited Loading

Codecov Report

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

codecov bot commented Jul 5, 2024 •

edited

Loading