[CLOUD_CONTAINER_DELETION] Core logic for container compaction #1646

ankagrawal · 2020-10-05T20:56:28Z

Container deletion logic in cloud.

codecov-commenter · 2020-10-05T21:43:19Z

Codecov Report

Merging #1646 into master will decrease coverage by 0.13%.
The diff coverage is 41.58%.

@@             Coverage Diff              @@
##             master    #1646      +/-   ##
============================================
- Coverage     73.56%   73.42%   -0.14%     
- Complexity     8639     8748     +109     
============================================
  Files           609      619      +10     
  Lines         47183    47827     +644     
  Branches       5962     6035      +73     
============================================
+ Hits          34708    35119     +411     
- Misses        10694    10903     +209     
- Partials       1781     1805      +24

Impacted Files	Coverage Δ	Complexity Δ
...om/github/ambry/cloud/CloudContainerCompactor.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...java/com/github/ambry/cloud/CloudRequestAgent.java	`88.57% <0.00%> (-2.61%)`	`9.00 <0.00> (ø)`
...hub/ambry/cloud/azure/AzureContainerCompactor.java	`20.45% <20.45%> (ø)`	`2.00 <2.00> (?)`
...com/github/ambry/cloud/ContainerDeletionEntry.java	`24.48% <24.48%> (ø)`	`3.00 <3.00> (?)`
...m/github/ambry/cloud/azure/CosmosDataAccessor.java	`61.15% <29.41%> (-2.49%)`	`40.00 <1.00> (+1.00)`	⬇️
...ithub/ambry/cloud/azure/AzureCloudDestination.java	`73.86% <54.54%> (-1.58%)`	`35.00 <6.00> (ø)`
.../java/com/github/ambry/cloud/StaticVcrCluster.java	`80.00% <60.00%> (-6.21%)`	`10.00 <1.00> (ø)`
...n/java/com/github/ambry/cloud/HelixVcrCluster.java	`72.97% <64.28%> (-2.44%)`	`12.00 <1.00> (+1.00)`	⬇️
...ithub/ambry/cloud/azure/AzureStorageCompactor.java	`71.08% <75.00%> (+0.17%)`	`25.00 <1.00> (ø)`
...ub/ambry/cloud/CloudContainerDeletionSyncTask.java	`78.57% <78.57%> (ø)`	`3.00 <3.00> (?)`
... and 85 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 40ccf67...bdc597b. Read the comment docs.

codecov-io · 2020-10-15T22:24:01Z

Codecov Report

Merging #1646 into master will decrease coverage by 57.94%.
The diff coverage is 0.00%.

@@              Coverage Diff              @@
##             master    #1646       +/-   ##
=============================================
- Coverage     73.65%   15.71%   -57.95%     
+ Complexity     8804     2088     -6716     
=============================================
  Files           629      630        +1     
  Lines         48371    48539      +168     
  Branches       6076     6094       +18     
=============================================
- Hits          35626     7626    -28000     
- Misses        10872    40476    +29604     
+ Partials       1873      437     -1436

Impacted Files	Coverage Δ	Complexity Δ
...main/java/com/github/ambry/config/CloudConfig.java	`0.00% <0.00%> (-100.00%)`	`0.00 <0.00> (-1.00)`
...java/com/github/ambry/cloud/CloudRequestAgent.java	`0.00% <0.00%> (-88.58%)`	`0.00 <0.00> (-9.00)`
...n/java/com/github/ambry/cloud/HelixVcrCluster.java	`0.00% <0.00%> (-75.00%)`	`0.00 <0.00> (-12.00)`
.../java/com/github/ambry/cloud/StaticVcrCluster.java	`0.00% <0.00%> (-86.21%)`	`0.00 <0.00> (-10.00)`
...c/main/java/com/github/ambry/cloud/VcrMetrics.java	`0.00% <0.00%> (-100.00%)`	`0.00 <0.00> (-2.00)`
.../com/github/ambry/cloud/VcrReplicationManager.java	`0.00% <0.00%> (-73.00%)`	`0.00 <0.00> (-15.00)`
...com/github/ambry/cloud/azure/AzureCloudConfig.java	`0.00% <0.00%> (-100.00%)`	`0.00 <0.00> (-1.00)`
...ithub/ambry/cloud/azure/AzureCloudDestination.java	`0.00% <0.00%> (-73.60%)`	`0.00 <0.00> (-35.00)`
...mbry/cloud/azure/AzureCloudDestinationFactory.java	`0.00% <0.00%> (-75.00%)`	`0.00 <0.00> (-5.00)`
.../github/ambry/cloud/azure/AzureCompactionUtil.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
... and 499 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 f401eb9...4dcc9f1. Read the comment docs.

lightningrob · 2020-10-20T01:00:27Z

Haven't reviewed yet, but Codecov says the diff coverages is just over 40%, so may want to beef that up.

ambry-api/src/main/java/com/github/ambry/config/CloudConfig.java

jsjtzyy · 2020-10-21T03:05:19Z

ambry-api/src/main/java/com/github/ambry/config/CloudConfig.java

+    cloudContainerCompactionIntervalHours = verifiableProperties.getInt(CLOUD_CONTAINER_COMPACTION_INTERVAL_HOURS, 24);
    cloudBlobCompactionStartupDelaySecs = verifiableProperties.getInt(CLOUD_BLOB_COMPACTION_STARTUP_DELAY_SECS, 600);
+    cloudContainerCompactionStartupDelaySecs =
+        verifiableProperties.getInt(CLOUD_CONTAINER_COMPACTION_STARTUP_DELAY_SECS, 600);


suggest using getIntInRange

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

jsjtzyy · 2020-10-21T04:47:12Z

ambry-cloud/src/main/java/com/github/ambry/cloud/azure/AzureCloudConfig.java

+    cosmosContainerDeletionBatchSize =
+        verifiableProperties.getInt(COSMOS_CONTAINER_DELETION_BATCH_SIZE, DEFAULT_COSMOS_CONTAINER_DELETION_BATCH_SIZE);
+    containerCompactionAbsPurgeLimit =
+        verifiableProperties.getInt(CONTAINER_COMPACTION_ABS_PURGE_LIMIT, DEFAULT_CONTAINER_COMPACTION_ABS_PURGE_LIMIT);
+    containerCompactionCosmosQueryLimit = verifiableProperties.getInt(CONTAINER_COMPACTION_COSMOS_QUERY_LIMIT,
+        DEFAULT_COSMOS_CONTAINER_DELETION_BATCH_SIZE);


same here, better to use getIntInRange

Its not 100% clear what the theoretical upper limit of this value should be. But it definitely shouldn't be less than 1. Made the changes accordingly.

lightningrob

Mostly looks good. A few concerns mentioned.

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

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

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

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

ambry-cloud/src/main/java/com/github/ambry/cloud/azure/CosmosDataAccessor.java

lightningrob · 2020-10-21T20:18:51Z

ambry-cloud/src/main/java/com/github/ambry/cloud/azure/CosmosDataAccessor.java

+      throws DocumentClientException {
+    String query = String.format(CONTAINER_BLOBS_QUERY, accountId, containerId);
+    SqlQuerySpec querySpec = new SqlQuerySpec(query);
+    FeedOptions feedOptions = new FeedOptions();


The code from here down is nearly identical to getDeadBlobs. Please refactor into a common class that take SqlQuerySpec as argument.

Also, since we aren't time bucketing the queries like dead blob compaction does, we could run into the same throttling issues that prompted the use of time bucketing there.

This is true. I plan to post a final follow up PR that would add two more optimizations

Do the container compaction in multiple threads.

Do a time based bucketing for deletion.

lightningrob · 2020-10-21T20:28:55Z

ambry-cloud/src/main/java/com/github/ambry/cloud/azure/CosmosDataAccessor.java

+    // Read the existing record
+    String id = CosmosContainerDeletionEntry.generateContainerDeletionEntryId(accountId, containerId);
+    String docLink = getContainerDeletionEntryDocumentLink(id);
+    RequestOptions options = getRequestOptions(id);


A lot of this logic is very similar to updateMetadata(). Possible to reuse/combine?

Will add as an action item for follow up PR.

ambry-cloud/src/test/resources/azure-test.properties

ambry-cloud/src/main/java/com/github/ambry/cloud/azure/AzureCompactionUtil.java

SophieGuo410

Overall lgtm. Added some minor comments.

SophieGuo410 · 2020-10-22T03:46:19Z

ambry-cloud/src/main/java/com/github/ambry/cloud/azure/AzureContainerCompactor.java

+   * Compact blobs of the deprecated container from cloud. This method is one of the two entry points in the
+   * {@link AzureContainerCompactor} class along with {@link AzureContainerCompactor#deprecateContainers(Collection, Collection)}.
+   * Note that this method is not thread safe as it is expected to run in a single thread.
+   */


minor: missing java doc for @param assignedPartitions

SophieGuo410 · 2020-10-22T03:56:33Z

ambry-cloud/src/test/java/com/github/ambry/cloud/azure/AzureTestUtils.java

@@ -13,9 +13,12 @@
 */
 package com.github.ambry.cloud.azure;

+import com.codahale.metrics.Timer;


minor: you can removed them since they are not been used.

SophieGuo410 · 2020-10-22T04:00:52Z

ambry-cloud/src/main/java/com/github/ambry/cloud/azure/CosmosContainerDeletionEntry.java

  static final String DELETE_PENDING_PARTITIONS_KEY = "deletePendingPartitions";
+  private static final String ID_KEY = "id";


SophieGuo410 · 2020-10-22T04:08:42Z

ambry-cloud/src/main/java/com/github/ambry/cloud/azure/AzureContainerCompactor.java

+        containerDeletionEntrySet.remove(containerDeletionEntry);
+        for (String partitionId : containerDeletionEntry.getDeletePendingPartitions()) {
+          try {
+            int blobCompactedCount =


Seems the blobCompactedCount never been used?

I will use it in future PR when I plan to add more metrics to the code.

SophieGuo410 · 2020-10-22T04:37:15Z

ambry-cloud/src/main/java/com/github/ambry/cloud/azure/AzureContainerCompactor.java

+      List<? extends PartitionId> assignedPartitions) throws CloudStorageException {
+    Set<CosmosContainerDeletionEntry> containerDeletionEntrySet =
+        requestAgent.doWithRetries(() -> cosmosDataAccessor.getDeprecatedContainers(containerDeletionQueryBatchSize),
+            "GetDeprectedContainers", null);


minor: GetDeprectedContainers -> GetDeprecatedContainers

lightningrob · 2020-11-03T04:48:58Z

ambry-cloud/src/main/java/com/github/ambry/cloud/azure/CosmosDataAccessor.java

  private static final String EXPIRED_BLOBS_QUERY = constructDeadBlobsQuery(CloudBlobMetadata.FIELD_EXPIRATION_TIME);
  private static final String DELETED_BLOBS_QUERY = constructDeadBlobsQuery(CloudBlobMetadata.FIELD_DELETION_TIME);
  private static final String CONTAINER_BLOBS_QUERY =
-      "SELECT TOP %d * FROM c WHERE c.accountId=%d and c.containerId=%d";
+      "SELECT TOP %d " + LIMIT_PARAM + " FROM c WHERE c.accountId=" + ACCOUNT_ID_PARAM + " and c.containerId="


I think Cosmos will reject this. "%d" needs to be removed, and you need the "*" before FROM.

lightningrob · 2020-11-03T04:49:54Z

ambry-cloud/src/main/java/com/github/ambry/cloud/azure/CosmosDataAccessor.java

  private static final String EXPIRED_BLOBS_QUERY = constructDeadBlobsQuery(CloudBlobMetadata.FIELD_EXPIRATION_TIME);
  private static final String DELETED_BLOBS_QUERY = constructDeadBlobsQuery(CloudBlobMetadata.FIELD_DELETION_TIME);
  private static final String CONTAINER_BLOBS_QUERY =
-      "SELECT TOP %d * FROM c WHERE c.accountId=%d and c.containerId=%d";
+      "SELECT TOP %d " + LIMIT_PARAM + " FROM c WHERE c.accountId=" + ACCOUNT_ID_PARAM + " and c.containerId="
+          + CONTAINER_ID_PARAM;
  private static final String BULK_DELETE_QUERY = "SELECT c._self FROM c WHERE c.id IN (%s)";
  private static final String DEPRECATED_CONTAINERS_QUERY =
      "SELECT TOP %d * from c WHERE c.deleted=false order by c.deleteTriggerTimestamp";


Can you change this to LIMIT_PARAM too? I missed it earlier.

lightningrob

Fix the Cosmos queries.

lightningrob · 2020-11-09T20:54:11Z

ambry-cloud/src/main/java/com/github/ambry/cloud/azure/CosmosDataAccessor.java

@@ -72,15 +72,15 @@
  private static final String LIMIT_PARAM = "@limit";
  private static final String ACCOUNT_ID_PARAM = "@accountId";
  private static final String CONTAINER_ID_PARAM = "@containerId";
+  private static final String MAX_ENTRIES_PARAM = "@maxEntries";


What's the difference between LIMIT and MAX_ENTRIES params?

Removed MAX_ENTRIES.

It doesn't look like it.

…en cloud and helix account service.

ankagrawal

Addressed all review comments.

ankagrawal · 2020-11-09T19:39:06Z

ambry-cloud/src/main/java/com/github/ambry/cloud/azure/CosmosDataAccessor.java

  private static final String EXPIRED_BLOBS_QUERY = constructDeadBlobsQuery(CloudBlobMetadata.FIELD_EXPIRATION_TIME);
  private static final String DELETED_BLOBS_QUERY = constructDeadBlobsQuery(CloudBlobMetadata.FIELD_DELETION_TIME);
  private static final String CONTAINER_BLOBS_QUERY =
-      "SELECT TOP %d * FROM c WHERE c.accountId=%d and c.containerId=%d";
+      "SELECT TOP %d " + LIMIT_PARAM + " FROM c WHERE c.accountId=" + ACCOUNT_ID_PARAM + " and c.containerId="


ankagrawal · 2020-11-09T19:39:16Z

ambry-cloud/src/main/java/com/github/ambry/cloud/azure/CosmosDataAccessor.java

  private static final String EXPIRED_BLOBS_QUERY = constructDeadBlobsQuery(CloudBlobMetadata.FIELD_EXPIRATION_TIME);
  private static final String DELETED_BLOBS_QUERY = constructDeadBlobsQuery(CloudBlobMetadata.FIELD_DELETION_TIME);
  private static final String CONTAINER_BLOBS_QUERY =
-      "SELECT TOP %d * FROM c WHERE c.accountId=%d and c.containerId=%d";
+      "SELECT TOP %d " + LIMIT_PARAM + " FROM c WHERE c.accountId=" + ACCOUNT_ID_PARAM + " and c.containerId="
+          + CONTAINER_ID_PARAM;
  private static final String BULK_DELETE_QUERY = "SELECT c._self FROM c WHERE c.id IN (%s)";
  private static final String DEPRECATED_CONTAINERS_QUERY =
      "SELECT TOP %d * from c WHERE c.deleted=false order by c.deleteTriggerTimestamp";


ankagrawal · 2020-11-23T07:23:49Z

ambry-cloud/src/main/java/com/github/ambry/cloud/azure/CosmosDataAccessor.java

@@ -72,15 +72,15 @@
  private static final String LIMIT_PARAM = "@limit";
  private static final String ACCOUNT_ID_PARAM = "@accountId";
  private static final String CONTAINER_ID_PARAM = "@containerId";
+  private static final String MAX_ENTRIES_PARAM = "@maxEntries";


Removed MAX_ENTRIES.

lightningrob

LGTM, thanks for making the changes.

SophieGuo410

LGTM!

…din#1646) * Initial implementation of Helix task to sync deleted containers between cloud and helix account service.

ankagrawal force-pushed the container_compaction branch from c7a0711 to 72e3f58 Compare October 15, 2020 20:28

ankagrawal force-pushed the container_compaction branch from 56735bb to 3874f5a Compare October 16, 2020 20:55

ankagrawal requested review from lightningrob, jsjtzyy and SophieGuo410 October 19, 2020 16:52

ankagrawal changed the title ~~[CLOUD_CONTAINER_DELETION] Core logic for container compaction (WIP)~~ [CLOUD_CONTAINER_DELETION] Core logic for container compaction Oct 19, 2020

jsjtzyy reviewed Oct 21, 2020

View reviewed changes

lightningrob requested changes Oct 21, 2020

View reviewed changes

SophieGuo410 reviewed Oct 22, 2020

View reviewed changes

ankagrawal force-pushed the container_compaction branch from 3f585ab to 4dcc9f1 Compare November 2, 2020 20:34

lightningrob reviewed Nov 3, 2020

View reviewed changes

lightningrob requested changes Nov 3, 2020

View reviewed changes

lightningrob reviewed Nov 9, 2020

View reviewed changes

Travis CI and others added 13 commits November 22, 2020 10:53

Initial implementation of Helix task to sync deleted containers betwe…

c847531

…en cloud and helix account service.

Fix tests and cleanup.

e1d180e

Fix BlobStoreCompactorTest

8d1c67c

Add unit tests

f541cda

Integration test (WIP)

847b6d5

Save ContainerDeletionEntry for deleted containers in cloud.

8dcd662

Fix tests

90bdcad

WIP Implement the container deletion logic

94002e5

Production code for container compaction.

27bd18f

Rebase and fix tests

c3dfdcd

Add logic to trigger container compaction in cloud.

8c720f1

Refactor compaction code and cleanup interfaces.

b235042

Update container on delete

fcb2f63

ankagrawal added 8 commits November 22, 2020 10:53

Add retries to azure operations.

9f3b507

Fix integration tests.

42924e2

cleanup

605d93c

Add cleanup after tests.

5737cd6

Address review comments

ac9b750

Address review comments

bbcda12

cleanup

0d6df8c

Address review comments

7f6c44c

ankagrawal force-pushed the container_compaction branch from d67105d to 7f6c44c Compare November 22, 2020 18:54

Add integration tests for cloud container compaction.

f23c798

ankagrawal commented Nov 23, 2020

View reviewed changes

lightningrob mentioned this pull request Nov 24, 2020

Refactor duplicate code in CosmosDataAccessor #1704

Open

Address review comments.

9fbd36a

lightningrob approved these changes Nov 24, 2020

View reviewed changes

SophieGuo410 approved these changes Nov 24, 2020

View reviewed changes

lightningrob merged commit ba801c3 into linkedin:master Nov 24, 2020

		static final String DELETE_PENDING_PARTITIONS_KEY = "deletePendingPartitions";
		private static final String ID_KEY = "id";

[CLOUD_CONTAINER_DELETION] Core logic for container compaction #1646

[CLOUD_CONTAINER_DELETION] Core logic for container compaction #1646

Conversation

ankagrawal commented Oct 5, 2020

codecov-commenter commented Oct 5, 2020

Codecov Report

codecov-io commented Oct 15, 2020 • edited Loading

Codecov Report

lightningrob commented Oct 20, 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

lightningrob 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

SophieGuo410 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

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

Choose a reason for hiding this comment

lightningrob 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

ankagrawal 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

lightningrob left a comment

Choose a reason for hiding this comment

SophieGuo410 left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 15, 2020 •

edited

Loading