Blobs get TTL upload should be uploaded to Cloud. #1165

zzmao · 2019-05-02T19:04:24Z

Fix blobs with ttlUpdate upload to cloud bug.
Refactored test Classes in ReplicationTest for their use in other modules.
Fix metric name typo in HelixVcrClusterMetrics.

codecov-io · 2019-05-02T19:18:16Z

Codecov Report

Merging #1165 into master will decrease coverage by 0.04%.
The diff coverage is 63.63%.

@@             Coverage Diff              @@
##             master    #1165      +/-   ##
============================================
- Coverage     70.03%   69.98%   -0.05%     
+ Complexity     5352     5348       -4     
============================================
  Files           426      426              
  Lines         32650    32656       +6     
  Branches       4142     4144       +2     
============================================
- Hits          22865    22855      -10     
- Misses         8653     8662       +9     
- Partials       1132     1139       +7

Impacted Files	Coverage Δ	Complexity Δ
...va/com.github.ambry.replication/ReplicaThread.java	`75.54% <0%> (ø)`	`63 <0> (ø)`	⬇️
...com.github.ambry.cloud/HelixVcrClusterMetrics.java	`100% <100%> (ø)`	`1 <0> (ø)`	⬇️
...c/main/java/com.github.ambry.cloud/VcrMetrics.java	`100% <100%> (ø)`	`2 <0> (ø)`	⬇️
...in/java/com.github.ambry.cloud/CloudBlobStore.java	`77.03% <62.5%> (+0.88%)`	`27 <0> (+3)`	⬆️
...java/com.github.ambry.network/BlockingChannel.java	`78.87% <0%> (-2.82%)`	`9% <0%> (-1%)`
...java/com.github.ambry.store/CompactionManager.java	`87.33% <0%> (-2.67%)`	`19% <0%> (ø)`
...in/java/com.github.ambry.store/BlobStoreStats.java	`71.75% <0%> (-1.65%)`	`104% <0%> (-1%)`
.../main/java/com.github.ambry.store/ScanResults.java	`81.25% <0%> (-1.57%)`	`16% <0%> (-1%)`
...ain/java/com.github.ambry.router/PutOperation.java	`90.86% <0%> (-0.72%)`	`107% <0%> (-2%)`
...src/main/java/com.github.ambry.commons/BlobId.java	`93.18% <0%> (-0.36%)`	`71% <0%> (-1%)`
... and 2 more

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 aaf8d92...03ced73. Read the comment docs.

lightningrob

I still need to do a more detailed review. My main suggestion so far is to rename the MockXXX classes to InMemoryXXX. Optional: consider making some of them static inner classes of a container class.
The test cases look very thorough.

ambry-cloud/src/test/java/com.github.ambry.cloud/CloudBlobStoreTest.java

ambry-cloud/src/main/java/com.github.ambry.cloud/CloudBlobStore.java

zzmao · 2019-05-03T18:08:34Z

Comment addressed. Let's try to merge it today.

ambry-cloud/src/main/java/com.github.ambry.cloud/CloudBlobStore.java

ambry-cloud/src/test/java/com.github.ambry.cloud/CloudBlobStoreTest.java

ambry-replication/src/test/java/com.github.ambry.replication/InMemoryStore.java

ambry-replication/src/test/java/com.github.ambry.replication/MockHost.java

lightningrob

Overall looks very good. Will approve after addressing a few remaining comments.

Fix blobs with ttlUpdate upload to cloud bug. Refactored test Classes in ReplicationTest for their use in other modules. Fix metric name typo in HelixVcrClusterMetrics.

Update expiration time is reflaced in TtlUpdate messageInfo. Revert CloudBlobStore#shouldUpload(). Add ttlUpdate() to endToEndCloudBackupTest.

zzmao requested review from cgtz and lightningrob May 2, 2019 19:04

lightningrob requested changes May 3, 2019

View reviewed changes

ambry-cloud/src/test/java/com.github.ambry.cloud/CloudBlobStoreTest.java Show resolved Hide resolved

ambry-cloud/src/main/java/com.github.ambry.cloud/CloudBlobStore.java Show resolved Hide resolved