-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Storage : Add support for Archive storage class #6685
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, just one documentation thing to change.
Also, the feature is not publicly available yet, so please hold off on merging until I have approval on that.
@@ -56,6 +56,9 @@ public StorageClass apply(String constant) { | |||
/** Standard storage class. */ | |||
public static final StorageClass STANDARD = type.createAndRegister("STANDARD"); | |||
|
|||
/** Archive storage class. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regional and Multi-regional are also considered deprecated now. Let's add that to the comment for each of those and also add a link to https://cloud.google.com/storage/docs/storage-classes which has the most up-to-date information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
.setNotFoundPage(NOT_FOUND_PAGE) | ||
.setLocation(LOCATION) | ||
.setLocationType(LOCATION_TYPE) | ||
.setStorageClass(ARCHIVE_STORAGE_CLASS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it typical to set all of these options while creating a bucket in Java? Just curious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to set all of these option but yes bucket name is required parameter see the api doc and example of create bucket
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, sounds good.
Codecov Report
@@ Coverage Diff @@
## master #6685 +/- ##
============================================
+ Coverage 46.35% 46.36% +<.01%
- Complexity 28005 28018 +13
============================================
Files 2613 2614 +1
Lines 288090 288195 +105
Branches 33783 33795 +12
============================================
+ Hits 133534 133610 +76
- Misses 144312 144334 +22
- Partials 10244 10251 +7
Continue to review full report at Codecov.
|
@tritone PTAL |
/** | ||
* Regional storage class (deprecated). | ||
* See:https://cloud.google.com/storage/docs/per-object-storage-class for details | ||
*/ | ||
public static final StorageClass REGIONAL = type.createAndRegister("REGIONAL"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's re-sort these as follows so that the non-deprecated classes come first: Standard, Nearline, Coldline, Archive, Regional, Multi-Regional, Durable Reduced Availability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
/** Multi-regional storage class. */ | ||
/** | ||
* Multi-regional storage class (deprecated). | ||
* See:https://cloud.google.com/storage/docs/per-object-storage-class for details |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This link redirects to https://cloud.google.com/storage/docs/storage-classes , so let's use that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
.setNotFoundPage(NOT_FOUND_PAGE) | ||
.setLocation(LOCATION) | ||
.setLocationType(LOCATION_TYPE) | ||
.setStorageClass(ARCHIVE_STORAGE_CLASS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, sounds good.
@tritone all the comment have been addressed PTAL |
Looks good to me! I will let you know when it's okay to merge (waiting until feature has been enabled without a flag). |
This PR should no longer be merged because of the repo split. @athakor , could you re-create this PR in https://github.com/googleapis/java-storage ? Please let me know if you run into any issues. Also, apologies for the delay on this-- we are now looking to merge the first week of Jan. |
done PR : googleapis/java-storage#19 |
Fixes #6578