feat: add support of customTime metadata #413

athakor · 2020-07-01T13:19:14Z

Towards #395

codecov · 2020-07-01T13:26:42Z

Codecov Report

Merging #413 into master will increase coverage by 1.38%.
The diff coverage is 91.66%.

@@             Coverage Diff              @@
##             master     #413      +/-   ##
============================================
+ Coverage     63.19%   64.58%   +1.38%     
- Complexity      613      617       +4     
============================================
  Files            32       32              
  Lines          5144     5232      +88     
  Branches        492      507      +15     
============================================
+ Hits           3251     3379     +128     
+ Misses         1729     1694      -35     
+ Partials        164      159       -5

Impacted Files	Coverage Δ	Complexity Δ
...c/main/java/com/google/cloud/storage/BlobInfo.java	`88.30% <90.00%> (+0.04%)`	`91.00 <1.00> (+3.00)`
...e/src/main/java/com/google/cloud/storage/Blob.java	`82.38% <100.00%> (+0.20%)`	`30.00 <0.00> (ø)`
...ain/java/com/google/cloud/storage/StorageImpl.java	`81.25% <0.00%> (+0.24%)`	`138.00% <0.00%> (ø%)`
...main/java/com/google/cloud/storage/BucketInfo.java	`83.97% <0.00%> (+1.96%)`	`86.00% <0.00%> (ø%)`
...gle/cloud/storage/testing/RemoteStorageHelper.java	`64.51% <0.00%> (+2.41%)`	`9.00% <0.00%> (ø%)`
...in/java/com/google/cloud/storage/PostPolicyV4.java	`86.33% <0.00%> (+23.67%)`	`5.00% <0.00%> (+1.00%)`

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 f0e945e...7ee53f8. Read the comment docs.

google-cloud-storage/clirr-ignored-differences.xml

athakor · 2020-07-06T09:13:41Z

@jkwlui

athakor · 2020-08-07T08:40:45Z

@frankyn @jkwlui friendly ping

frankyn

I have a few nits for documentation, otherwise LGTM.

google-cloud-storage/src/main/java/com/google/cloud/storage/BlobInfo.java

google-cloud-storage/clirr-ignored-differences.xml

elharo · 2020-08-25T13:43:28Z

google-cloud-storage/src/main/java/com/google/cloud/storage/BlobInfo.java

@@ -260,6 +261,9 @@ static CustomerEncryption fromPb(StorageObject.CustomerEncryption customerEncryp
     */
    public abstract Builder setCrc32c(String crc32c);

+    /** Sets the custom time for an object. */
+    public abstract Builder setCustomTime(Long customTime);


Why is this abstract? If it's concrete it wouldn't be a breaking change.

It's the development model we've used over the years. It was designed before my time and folks who worked on the library have since left the team.

We could update it to concrete method instead but we would start changing development flow. Maybe that's a good thing, but I'd prefer to prevent changing development flow until we have a new design that fits into that model.

The pot roast principle applies:

https://www.psychologytoday.com/us/blog/thinking-makes-it-so/201402/the-pot-roast-principle

If it's implemented, it could be implemented to raise an exception of it not being implemented. Seems a little clunky, but would remove the need to create a breaking change.

Thanks for raising the simple questions @elharo, I haven't applied this train of thought to this problem, and I appreciate the feedback.

@athakor please implement this method instead of using abstract to prevent the breaking change. Instead raise an exception of unimplemented if called directly.

athakor · 2020-08-26T06:58:52Z

@frankyn PTAL

JesseLovelace · 2020-08-26T23:10:18Z

google-cloud-storage/src/main/java/com/google/cloud/storage/BlobInfo.java

@@ -260,6 +261,22 @@ static CustomerEncryption fromPb(StorageObject.CustomerEncryption customerEncryp
     */
    public abstract Builder setCrc32c(String crc32c);

+    /**
+     * Sets the custom time for an object. Once set it can't be unset and only changed to a custom
+     * datetime in the future. If the custom_time must be unset, you must either perform a rewrite


Don't use "custom_time" in this context, say "custom time" instead. Replace "If the custom_time must be unset," with "To unset the custom time,"

JesseLovelace · 2020-08-26T23:10:32Z

google-cloud-storage/src/main/java/com/google/cloud/storage/BlobInfo.java

+     * datetime in the future. If the custom_time must be unset, you must either perform a rewrite
+     * operation or upload the data again.
+     *
+     * <p>Example of setting the custom_time.


custom time instead of custom_time

athakor · 2020-08-27T14:30:31Z

@frankyn @JesseLovelace All the comments have been addressed PTAL

elharo, PR addressed the remaining comment about the breaking change while also raising an exception of unimplemented if used directly. Dismissing to merge.

…rage into addolmexception

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 1, 2020

athakor requested a review from frankyn July 1, 2020 13:19

elharo previously requested changes Jul 1, 2020

View reviewed changes

google-cloud-storage/clirr-ignored-differences.xml Outdated Show resolved Hide resolved

athakor added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 9, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 9, 2020

frankyn added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 10, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 10, 2020

athakor added kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. labels Jul 14, 2020

yoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jul 14, 2020

frankyn added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 14, 2020

athakor added kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. labels Jul 16, 2020

yoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jul 16, 2020

athakor force-pushed the storage-blob-customTime branch from 70aa315 to 6e36ce5 Compare July 24, 2020 12:43

athakor added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 5, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 5, 2020

athakor added 2 commits August 7, 2020 14:04

feat: support customTime metadata

7ed367e

feat: add javaDoc

50df38f

athakor force-pushed the storage-blob-customTime branch from 6e36ce5 to 50df38f Compare August 7, 2020 08:35

product-auto-label bot added the api: storage Issues related to the googleapis/java-storage API. label Aug 21, 2020

frankyn removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 24, 2020

frankyn suggested changes Aug 24, 2020

View reviewed changes

google-cloud-storage/src/main/java/com/google/cloud/storage/BlobInfo.java Outdated Show resolved Hide resolved

google-cloud-storage/clirr-ignored-differences.xml Outdated Show resolved Hide resolved

elharo reviewed Aug 25, 2020

View reviewed changes

feat: address few nits

9f0bbaa

JesseLovelace reviewed Aug 26, 2020

View reviewed changes

feat: fix the api breaking change

ed62141

Add exception to avoid breaking change

17ed9c5

frankyn approved these changes Aug 27, 2020

View reviewed changes

frankyn requested a review from elharo August 27, 2020 21:51

Add exception to avoid breaking change

fe2ffbf

Merge branch 'storage-blob-customTime' of github.com:athakor/java-sto…

7ee53f8

…rage into addolmexception

JesseLovelace merged commit 6f4585e into googleapis:master Aug 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support of customTime metadata #413

feat: add support of customTime metadata #413

athakor commented Jul 1, 2020

codecov bot commented Jul 1, 2020 •

edited

Loading

athakor commented Jul 6, 2020

athakor commented Aug 7, 2020

frankyn left a comment

elharo Aug 25, 2020

frankyn Aug 25, 2020

elharo Aug 25, 2020

frankyn Aug 25, 2020

frankyn Aug 26, 2020

athakor Aug 27, 2020

athakor commented Aug 26, 2020

JesseLovelace Aug 26, 2020

athakor Aug 27, 2020

JesseLovelace Aug 26, 2020

athakor Aug 27, 2020

athakor commented Aug 27, 2020

feat: add support of customTime metadata #413

feat: add support of customTime metadata #413

Conversation

athakor commented Jul 1, 2020

codecov bot commented Jul 1, 2020 • edited Loading

Codecov Report

athakor commented Jul 6, 2020

athakor commented Aug 7, 2020

frankyn left a 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

athakor commented Aug 26, 2020

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

athakor commented Aug 27, 2020

codecov bot commented Jul 1, 2020 •

edited

Loading