Add parameter to GET /accounts API to ignore containers information #2998

Arun-LinkedIn · 2025-01-29T17:24:00Z

Summary

Add parameter in GET /accounts API to ignore containers information. This is useful for fetching fields of Account (such as quotaResourceType, aclInheritedByContainer) without overhead of containers information.

Testing Done

Tested in locally deployed Ambry

With container information

> curl "http://localhost:1174/accounts" -H "x-ambry-target-account-name: named-blob-sandbox" | jq .
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   889  100   889    0     0   3108      0 --:--:-- --:--:-- --:--:--  3108
{
  "accounts": [
    {
      "snapshotVersion": 0,
      "lastModifiedTime": 0,
      "status": "ACTIVE",
      "aclInheritedByContainer": false,
      "quotaResourceType": "ACCOUNT",
      "accountId": 101,
      "accountName": "named-blob-sandbox",
      "containers": [
        {
          "status": "ACTIVE",
          "deleteTriggerTime": 0,
          "description": "This is a container for the blobs without specifying a target account and container when they are put",
          "encrypted": false,
          "cacheable": true,
          "backupEnabled": false,
          "mediaScanDisabled": false,
          "paranoidDurabilityEnabled": false,
          "replicationPolicy": null,
          "ttlRequired": true,
          "securePathRequired": false,
          "namedBlobMode": "OPTIONAL",
          "accessControlAllowOrigin": "",
          "contentTypeWhitelistForFilenamesOnDownload": [],
          "lastModifiedTime": 0,
          "snapshotVersion": 0,
          "cacheTtlInSecond": null,
          "userMetadataKeysToNotPrefixInResponse": [],
          "containerId": 8,
          "containerName": "container-a",
          "previouslyEncrypted": false,
          "overrideAccountAcl": false,
          "version": 2
        }
      ],
      "version": 1
    }
  ]
}

Without container information

> curl "http://localhost:1174/accounts" -H "x-ambry-target-account-name: named-blob-sandbox" -H "x-ambry-ignore-containers:true" | jq .
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   200  100   200    0     0   2387      0 --:--:-- --:--:-- --:--:--  2380
{
  "accounts": [
    {
      "snapshotVersion": 0,
      "lastModifiedTime": 0,
      "status": "ACTIVE",
      "aclInheritedByContainer": false,
      "quotaResourceType": "ACCOUNT",
      "accountId": 101,
      "accountName": "named-blob-sandbox",
      "version": 1
    }
  ]
}

codecov-commenter · 2025-01-29T17:32:57Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.03%. Comparing base (52ba813) to head (0008782).
Report is 167 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2998      +/-   ##
============================================
+ Coverage     64.24%   70.03%   +5.79%     
- Complexity    10398    12136    +1738     
============================================
  Files           840      884      +44     
  Lines         71755    74786    +3031     
  Branches       8611     8966     +355     
============================================
+ Hits          46099    52378    +6279     
+ Misses        23004    19682    -3322     
- Partials       2652     2726      +74

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

allenaverbukh

minor questions + style .. if you think they can be disregarded -- let me know and i can approve

allenaverbukh · 2025-01-29T18:30:03Z

ambry-frontend/src/main/java/com/github/ambry/frontend/GetAccountsHandler.java

maybe serializedAccounts might be better variable name?

Sure. Done..

allenaverbukh · 2025-01-29T18:30:30Z

ambry-frontend/src/test/java/com/github/ambry/frontend/GetAccountsHandlerTest.java

+      RestResponseChannel restResponseChannel = new MockRestResponseChannel();
+      ReadableStreamChannel channel = sendRequestGetResponse(request, restResponseChannel);
+      assertNotNull("There should be a response", channel);
+      Assert.assertNotNull("Date has not been set", restResponseChannel.getHeader(RestUtils.Headers.DATE));


why does line 181 not have "Assert." like line 182

Yeah.. made it consistent

allenaverbukh · 2025-01-29T18:33:40Z

ambry-api/src/main/java/com/github/ambry/account/AccountCollectionSerde.java

+   */
+  public static byte[] serializeAccountsInJsonNoContainers(Collection<Account> accounts) throws IOException {
+    Map<String, Collection<Account>> resultObj = new HashMap<>();
+    resultObj.put(ACCOUNTS_KEY, accounts);


im still new to concurrency but assuming concurrency is involved, do you see any issues that could occur with having a non-concurrent hashmap here? will the different threads not overwrite anything or have race condition ?

There is no concurrency involved with this method. This map is created in this method, which makes it a local variable to this method. If we have multiple threads executing this method, they would just create multiple map objects.

justinlin-linkedin · 2025-01-29T22:45:46Z

ambry-frontend/src/main/java/com/github/ambry/frontend/GetAccountsHandler.java

+          boolean ignoreContainers =
+              RestUtils.getBooleanHeader(restRequest.getArgs(), RestUtils.Headers.IGNORE_CONTAINERS, false);
+          if (ignoreContainers) {
+            serialized = AccountCollectionSerde.serializeAccountsInJsonNoContainers(getAccounts());
+          } else {
+            serialized = AccountCollectionSerde.serializeAccountsInJson(getAccounts());
+          }


can we merge these two methods to one method with extra parameter? Something like

serialized = AccountCollectionSerde.serializeAccountsInJson(getAccountsI(), RestUtils.getBooleanHeader(restRequest.getArgs(), RestUtils.Headers.IGNORE_CONTAINERS, false));

Basically move the boolean logic to AccountCollectionSerde.

Sure. Done..

justinlin-linkedin · 2025-01-29T22:47:30Z

ambry-api/src/main/java/com/github/ambry/account/AccountCollectionSerde.java

+   */
+  public static byte[] serializeAccountsInJsonNoContainers(Collection<Account> accounts) throws IOException {
+    Map<String, Collection<Account>> resultObj = new HashMap<>();
+    resultObj.put(ACCOUNTS_KEY, accounts);


There is no concurrency involved with this method. This map is created in this method, which makes it a local variable to this method. If we have multiple threads executing this method, they would just create multiple map objects.

Add parameter to /accounts API to ignore containers information

9e25343

Arun-LinkedIn requested review from justinlin-linkedin and allenaverbukh January 29, 2025 17:24

allenaverbukh reviewed Jan 29, 2025

View reviewed changes

justinlin-linkedin reviewed Jan 29, 2025

View reviewed changes

Arun Bhima added 2 commits January 29, 2025 19:25

Address comments

c4a9088

Address another comment

0008782

allenaverbukh approved these changes Jan 30, 2025

View reviewed changes

justinlin-linkedin approved these changes Jan 30, 2025

View reviewed changes

Arun-LinkedIn merged commit f7d2c88 into linkedin:master Jan 30, 2025
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add parameter to GET /accounts API to ignore containers information #2998

Add parameter to GET /accounts API to ignore containers information #2998

Arun-LinkedIn commented Jan 29, 2025

codecov-commenter commented Jan 29, 2025 •

edited

Loading

allenaverbukh left a comment

allenaverbukh Jan 29, 2025

Arun-LinkedIn Jan 30, 2025

allenaverbukh Jan 29, 2025

Arun-LinkedIn Jan 30, 2025

allenaverbukh Jan 29, 2025

justinlin-linkedin Jan 29, 2025

Arun-LinkedIn Jan 30, 2025

justinlin-linkedin Jan 29, 2025

Arun-LinkedIn Jan 30, 2025

justinlin-linkedin Jan 29, 2025

Add parameter to GET /accounts API to ignore containers information #2998

Add parameter to GET /accounts API to ignore containers information #2998

Conversation

Arun-LinkedIn commented Jan 29, 2025

Summary

Testing Done

codecov-commenter commented Jan 29, 2025 • edited Loading

Codecov Report

allenaverbukh 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

codecov-commenter commented Jan 29, 2025 •

edited

Loading