Skip to content
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

Closed
wants to merge 4 commits into from

Conversation

athakor
Copy link
Contributor

@athakor athakor commented Nov 4, 2019

Fixes #6578

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 4, 2019
Copy link
Contributor

@tritone tritone left a 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. */
Copy link
Contributor

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.

Copy link
Contributor Author

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)
Copy link
Contributor

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.

Copy link
Contributor Author

@athakor athakor Nov 7, 2019

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, sounds good.

@codecov
Copy link

codecov bot commented Nov 7, 2019

Codecov Report

Merging #6685 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             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
Impacted Files Coverage Δ Complexity Δ
...in/java/com/google/cloud/storage/StorageClass.java 88.23% <100%> (+0.73%) 3 <0> (ø) ⬇️
...google/cloud/bigquery/StandardTableDefinition.java 86.76% <0%> (-5.43%) 14% <0%> (ø)
...main/java/com/google/cloud/storage/BucketInfo.java 83.18% <0%> (-1.19%) 84% <0%> (ø)
...a/com/google/cloud/bigquery/RangePartitioning.java 75% <0%> (ø) 9% <0%> (?)
...com/google/cloud/bigquery/QueryParameterValue.java 72.72% <0%> (+0.35%) 46% <0%> (ø) ⬇️
...om/google/cloud/bigquery/LoadJobConfiguration.java 90.15% <0%> (+0.44%) 52% <0%> (+2%) ⬆️
...m/google/cloud/bigquery/QueryJobConfiguration.java 77.14% <0%> (+0.74%) 56% <0%> (+2%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8da3df...c2e02c8. Read the comment docs.

@athakor
Copy link
Contributor Author

athakor commented Nov 7, 2019

@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");
Copy link
Contributor

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

Copy link
Contributor Author

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
Copy link
Contributor

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.

Copy link
Contributor Author

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, sounds good.

@athakor
Copy link
Contributor Author

athakor commented Nov 12, 2019

@tritone all the comment have been addressed PTAL

@tritone
Copy link
Contributor

tritone commented Nov 13, 2019

@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).

@chingor13 chingor13 added the api: storage Issues related to the Cloud Storage API. label Dec 6, 2019
@tritone
Copy link
Contributor

tritone commented Dec 20, 2019

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.

@athakor
Copy link
Contributor Author

athakor commented Dec 27, 2019

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

@athakor athakor closed this Dec 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Storage: Add support for Archive storage class
4 participants