Use rename LRO on HN buckets #1140
Conversation
|
/gcbrun |
|
/gcbrun |
1 similar comment
|
/gcbrun |
|
/gcbrun |
3 similar comments
|
/gcbrun |
|
/gcbrun |
|
/gcbrun |
gcs/src/test/java/com/google/cloud/hadoop/fs/gcs/GoogleHadoopFileSystemIntegrationTest.java
Show resolved
Hide resolved
| @@ -173,6 +174,10 @@ public void close() { | |||
| } | |||
| } | |||
|
|
|||
| public void renameHnFolder(URI src, URI dst) throws IOException { | |||
There was a problem hiding this comment.
Wouldn't this be taken care by ForwardingGoogleCloudStorage?
There was a problem hiding this comment.
Change in made in the ForwardingGoogleCloudStroage as well. Not sure if I understood the comment.
There was a problem hiding this comment.
I meant to say why we need to override it in GCSClientImpl? I don't see implementation of it is not deviating from ForwardingGoogleCloudStorage implementation.
| @@ -122,6 +126,9 @@ public class GoogleCloudStorageImpl implements GoogleCloudStorage { | |||
| .setEnsureEmptyObjectsMetadataMatch(false) | |||
| .build(); | |||
|
|
|||
| private static Cache<String, Boolean> cache = | |||
There was a problem hiding this comment.
Note that we need to check if the bucket is HN enabled before we use rename operation. We are caching this information, rather than calling this before each rename.
There was a problem hiding this comment.
I understand that, I meant to say we can improve of variable name.
There was a problem hiding this comment.
fair point.
| @@ -444,8 +456,19 @@ public void createBucket(String bucketName, CreateBucketOptions options) throws | |||
| Bucket bucket = | |||
| new Bucket() | |||
| .setName(bucketName) | |||
| .setHierarchicalNamespace( | |||
There was a problem hiding this comment.
-
This flow uses Apiary client to make calls to gcs. HN buckets have visibility in Apiary also?
-
We we are adding another attribute(
setHierarchicalNamespace) to bucket object. What could be the potential impact on existing customers who aren't using HN?
There was a problem hiding this comment.
This is the createBucket flow. We do not support creating HN bucket from connector since we wont know if customer expect to create an HN bucket or non-HN bucket. But we need it for testing purposed. This change is just for testing since it need to create an HN bucket to test.
There was a problem hiding this comment.
one way of removing any impact on existing customer is to put it under options.getHierarchicalNamespaceEnabled() for eg:
Bucket bucket =
new Bucket()
.setName(bucketName)
.setLocation(options.getLocation())
.setStorageClass(options.getStorageClass());
if(options.getHierarchicalNamespaceEnabled()) {
bucket =
new Bucket()
.setName(bucketName)
.setLocation(options.getLocation())
.setHierarchicalNamespace(.....
.setStorageClass(options.getStorageClass());
bucket.setIamConfiguration(
new Bucket.IamConfiguration()
.setUniformBucketLevelAccess(
new Bucket.IamConfiguration.UniformBucketLevelAccess().setEnabled(true)));
}
There was a problem hiding this comment.
as discussed offline this will be fixed in follow up PR cc: @arunkumarchacko
gcsio/src/main/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageImpl.java
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| public void renameHnFolder(URI src, URI dst) throws IOException { |
There was a problem hiding this comment.
should we validate that both src and dst have same Authority i.e. bucket?
There was a problem hiding this comment.
Good point. I will fix this in a later change.
gcsio/src/main/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageOptions.java
Show resolved
Hide resolved
|
/gcbrun |
2 similar comments
|
/gcbrun |
|
/gcbrun |
|
/gcbrun |
arunkumarchacko
merged commit 01f76d3
into
GoogleCloudDataproc:branch-3.0.x
No description provided.