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

HDDS-8214. Recon - OM DB Insights - Key Level Info #4516

Merged
merged 24 commits into from
May 25, 2023

Conversation

devmadhuu
Copy link
Contributor

@devmadhuu devmadhuu commented Apr 3, 2023

Few new APIs will be exposed in Recon for below information related to keys:
Key Level Info (Tab 2): A new tab will be added in Recon to consume data from newly added APIs

  1. Number of open keys.
  2. Number of pending delete keys.
  3. Containers in DELETED state in SCM and present in OM, this info will be used in another API (api/v1/<containerId>/keys) as input to get list of keys mapped to such containers.

https://issues.apache.org/jira/browse/HDDS-8214

For point 1 and point 2 above - API request endpoint:

GET Request - "api/v1/keys/open?limit=10&prevKey=10"
First time don't send preKey param. In 2nd page call, send preKey value as "lastKey" value got in 1st response.
Response:

{
    lastKey : 11,
    replicatedTotal: 13824,
    unreplicatedTotal: 4608,
    entities: [
    {
        path: “/vol1/bucket1/key1”,
        keyState: “Open”,
        inStateSince: 1667564193026,
        size: 1024,
        replicatedSize: 3072,
        unreplicatedSize: 1024,
        replicationType: RATIS,
        replicationFactor: THREE  
    }.
   {
        path: “/vol1/bucket1/key2”,
        keyState: “Open”,
        inStateSince: 1667564193026,
        size: 512,
        replicatedSize: 1536,
        unreplicatedSize: 512,
        replicationType: RATIS,
        replicationFactor: THREE 
    }
  ]
}

GET Request for pending delete keys- "api/v1/keys/deletePending/keys?limit=10&prevKey=10"
GET Request for pending delete directories- "api/v1/keys/deletePending/dirs?limit=10&prevKey=10"
First time don't send preKey param. In 2nd page call, send preKey value as "lastKey" value got in 1st response.
Response:

{
       lastKey : 10,
      "replicatedTotal": -1530804718628866300,
      "unreplicatedTotal": -1530804718628866300,
      "deletedkeyinfo": [
        {
          "omKeyInfoList": [
            {
              "metadata": {},
              "objectID": 0,
              "updateID": 0,
              "parentObjectID": 0,
              "volumeName": "sampleVol",
              "bucketName": "bucketOne",
              "keyName": "key_one",
              "dataSize": -1530804718628866300,
              "keyLocationVersions": [],
              "creationTime": 0,
              "modificationTime": 0,
              "replicationConfig": {
                "replicationFactor": "ONE",
                "requiredNodes": 1,
                "replicationType": "STANDALONE"
              },
              "fileChecksum": null,
              "fileName": "key_one",
              "acls": [],
              "path": "0/key_one",
              "file": false,
              "latestVersionLocations": null,
              "replicatedSize": -1530804718628866300,
              "fileEncryptionInfo": null,
              "objectInfo": "OMKeyInfo{volume='sampleVol', bucket='bucketOne',
              key='key_one', dataSize='-1530804718628866186', creationTime='0',
              objectID='0', parentID='0', replication='STANDALONE/ONE',
              fileChecksum='null}",
              "updateIDset": false
            }
          ]
        }
      ],
      "status": "OK"
    }

GET Request for 3rd point (Containers in DELETED state in SCM and present in OM) - "api/v1/containers/mismatch/deleted?limit=10&prevKey=10"
Response:

[
  {
    "containerId": 2,
    "numberOfKeys": 2,
    "pipelines": []
  }
]

How was this patch tested?

This patch was tested using Junit test case and postman as well to verify response.

@devmadhuu devmadhuu marked this pull request as ready for review April 3, 2023 05:44
@devmadhuu devmadhuu marked this pull request as draft April 3, 2023 05:45
@kerneltime kerneltime requested review from dombizita and smengcl April 3, 2023 16:15
@devmadhuu devmadhuu marked this pull request as ready for review April 4, 2023 14:53
@kerneltime kerneltime self-requested a review April 10, 2023 16:33
@kerneltime
Copy link
Contributor

@SaketaChalamchala can you please take a look?

Map<Long, ContainerMetadata> omContainers =
reconContainerMetadataManager.getContainers(-1, 0);
List<ContainerInfo> deletedStateSCMContainers =
containerManager.getContainers(HddsProtos.LifeCycleState.DELETED);
Copy link
Contributor

Choose a reason for hiding this comment

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

The code does not check if the containerManager are null before calling methods on them, which could result in a NullPointerException if they are null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not needed to be checked for an extra null check because this is guice framework injection mechanism and used as standard in whole ozone codebase.


omContainerIdsMappedToDeletedSCMContainers.forEach(containerId -> {
Response keysForContainer =
containerEndpoint.getKeysForContainer(containerId, limit,
Copy link
Contributor

Choose a reason for hiding this comment

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

The code does not check if the containerEndpoint are null before calling methods on them, which could result in a NullPointerException if they are null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not needed to be checked for an extra null check because this is guice framework injection mechanism and used as standard in whole ozone codebase.

* {
* path: “/vol1/bucket1/key1”,
* keyState: “Open”,
* inStateSince: 1667564193026,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible for us to improve the timestamp format for inStateSince to be more redabale like :-

  "inStateSince": "2023-04-14T12:00:00Z",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Backend at server side should always use timestamp in epoch format and client/UI should convert into local timezone format, so providing in epcoh is standard way.

omMetadataManager.getOpenKeyTable(layout);
try (
TableIterator<String, ? extends Table.KeyValue<String, OmKeyInfo>>
keyIter = openKeyTable.iterator()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to close the keyIter in case an exception is thrown so as to avoid resource locking by the iterator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this iterator is opened in try with block and any exception thrown will not be a leak which is taken care by java. Standard way to use in many other places in Ozone code base.

omMetadataManager.getDeletedDirTable();
try (
TableIterator<String, ? extends Table.KeyValue<String, OmKeyInfo>>
keyIter = deletedDirTable.iterator()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Close the keyIter here as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this iterator is opened in try with block and any exception thrown will not be a leak which is taken care by java. Standard way to use in many other places in Ozone code base.

try (
TableIterator<String, ? extends Table.KeyValue<String,
RepeatedOmKeyInfo>>
keyIter = deletedTable.iterator()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Close the keyIter here as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this iterator is opened in try with block and any exception thrown will not be a leak which is taken care by java. Standard way to use in many other places in Ozone code base.

* }
*/
@GET
@Path("openkeyinfo")

Choose a reason for hiding this comment

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

Can we follow proper Rest naming standards. Something like
/api/v1/keys/open
/api/v1/keys/deletePending
/api/v1/containers/mismatch or whatever is appropriate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@krishnaasawa1 - Thanks for review, however I believe above api of "openkeyinfo" is accessed using same naming standards like "/api/vi/omdbinsight/openkeyinfo", because "api/v1" is already a prefix set at all APIs top from jetty server.

Choose a reason for hiding this comment

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

That I understand I am talking about Rest Naming convention. openkeyinfo or pendingfordeletionkeyinfo is not something appropriate name or path to expose in Rest API . APIs need to be Resource Name based with Actions if required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@krishnaasawa1 - Okay, I think I understood what you meant. I'll follow the path naming convention as suggested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@krishnaasawa1 - Pls re-review. Changes pushed for API naming convention.

openKeyInsightInfo.getNonFSOKeyInfoList();
boolean isLegacyBucketLayout = true;
boolean recordsFetchedLimitReached = false;
List<KeyEntityInfo> fsoKeyInfoList = openKeyInsightInfo.getFsoKeyInfoList();

Choose a reason for hiding this comment

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

The logic for both Opentable and deleteTable seems similar with Table fetch, using iterator , creating KeyEntity and adding to Response and updating overall un replication size and replication size.
Is there a way to optimise and use common code for Table iterator and updates for Open as well deleted case.
Also for OMkeyInfo is transformed to KeyEntity object for Response in case on OpenKey as well as DeletedDir , but for deletedKey repeatedOmKeyInfo is directly returned which has OMkeyInfo List .
So not sure about this logic but either in other case we could OMkeyInfo or also transform repeatedOmKeyInfo to Keyentity List

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@krishnaasawa1 thanks for your review, so on you below point:

  1. Is there a way to optimise and use common code for Table iterator and updates for Open as well deleted case.
    • Both Open Key and Deleted key responses are different , though they are prepared after iteration from their respective tables. In openKey API response, logic is different as we have to consider both OBS/LEGACY and FSO buckets because the tsandard interface provided by omMetadataManager accepts bucketlayout and provides the reference of "openKeyTable" or "openFileTable" respectively and their iterators return "Table<String, OmKeyInfo>" , however in case of deletedKeys API, omMetadataManager doesn't have any standard interface for two different tables ("deletedTable" and "deletedDirectoryTable" ), so we don't have to provide bucket layout as an argument and we have to fetch data from "deletedTable" for keys and need to fetch data from "deletedDirectoryTable" for directories which is done separately in two different methods due to their tables provide different iterators like "Table<String, RepeatedOmKeyInfo>" and "Table.KeyValue<String, OmKeyInfo>>", so you can see here, iterator logic cannot be same. because one provides RepeatedKeyInfo and another just single OmKeyInfo. And that is the reason we have different response for different APIs as per design doc we discussed.

Choose a reason for hiding this comment

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

Thanks for detailed response. It's okay to keep same if we can't optimize.

return pendingForDeletionKeyInfo;
}

private KeyInsightInfoResp getPendingForDeletionKeyInfo(

Choose a reason for hiding this comment

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

Return KeyInsightInfoResp , Same object passed as input param KeyInsightInfoResp is returned post modification. Return seems could be void.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@krishnaasawa1 thanks for review, however this returned is not void, this KeyInsightInfoResp is passed as a empty object to populate pendingForDeltionKeys to getPendingForDeletionKeyInfo method and later after populating it , it is being passed as reference to "getPendingForDeletionDirInfo" method.

Choose a reason for hiding this comment

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

What I meant as object is passed after creating new, no need to return back KeyInsightInfoResp as calling method already have reference of same object as it only has passed as input param . It's okay to keep , if required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@krishnaasawa1 - Thanks for elaborating, I understood your point. Will make a change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@krishnaasawa1 - Pls re-review. Changes pushed for making methods return as void.

* 5. Amount of data mapped to pending delete keys in legacy/OBS buckets and
* pending delete files in FSO buckets.
*/
@Path("/omdbinsight")

Choose a reason for hiding this comment

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

We may not need /omdbinsight in path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We may not need /omdbinsight in path

@krishnaasawa1 this is the general convention that what this whole endpoint is about and what the APIs contains regarding, we follow this convention in other endpoints also like "MetricsProxyEndpoint", "NSSummaryEndpoint", "ClusterStateEndpoint" etc.

Choose a reason for hiding this comment

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

Those belong to /metrics , /namespace and /clusterstate APIs , so here new APIs are for Containers and Keys. Based on https://ozone.apache.org/docs/current/interface/reconapi.html it looks more appropriate to keep them in Containers and Keys category respectively as information is regarding that instead of creating appending new categories omdbinsight , scminsights , insights etc in path APIs. Those APIs can map in UI accordingly to whatever insights we want to display to user. e.g namespace apis are called from insights UI page.

Copy link
Contributor Author

@devmadhuu devmadhuu Apr 19, 2023

Choose a reason for hiding this comment

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

Those belong to /metrics , /namespace and /clusterstate APIs , so here new APIs are for Containers and Keys. Based on https://ozone.apache.org/docs/current/interface/reconapi.html it looks more appropriate to keep them in Containers and Keys category respectively as information is regarding that instead of creating appending new categories omdbinsight , scminsights , insights etc in path APIs. Those APIs can map in UI accordingly to whatever insights we want to display to user. e.g namespace apis are called from insights UI page.

This point can be debatable, I have removed the "omdbinsight" from top path of API, however based on your earlier comment, I changed the API path to "/containers/keys/mismatch" for the API which retrieves set of keys/files/dirs which are mapped to

  • containers in DELETED state in SCM. so now based on above comment, we should not keep the API path as "/containers/keys/mismatch", because this API doesn't retrieve containers, so I have changed the API path by adding top level as "/keys" and rename the path like "/containers/mismatch". If you have some specific path as suggestion, pls provide as suggestion , so that I can do exactly that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, I have moved "/containers/mismatch/keys" to ContainerEndPoint, pls have a re-look.

/** This method retrieves set of keys/files/dirs which are mapped to
* containers in DELETED state in SCM. */
@GET
@Path("/keys/deletedContainers")

Choose a reason for hiding this comment

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

This is case when containers is in OM and not in SCM (deleted) , then only keys will be available.
Relevant resource path seems as /containers/mismatch/keys

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is case when containers is in OM and not in SCM (deleted) , then only keys will be available. Relevant resource path seems as /containers/mismatch/keys

@krishnaasawa1 I have done the change as per the suggestion.

map -> deletedStateSCMContainerIds.contains(map.getKey()))
.map(map -> map.getKey()).collect(Collectors.toList());

omContainerIdsMappedToDeletedSCMContainers.forEach(containerId -> {

Choose a reason for hiding this comment

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

further steps only applicable if omContainerIdsMappedToDeletedSCMContainers non empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

further steps only applicable if omContainerIdsMappedToDeletedSCMContainers non empty.

@krishnaasawa1 Not sure what you meant, I believe if this list is empty, then it will not execute and iterate forEach block.

openKeyInsightInfo.setReplicatedTotal(
openKeyInsightInfo.getReplicatedTotal() +
keyEntityInfo.getReplicatedSize());
boolean added =
Copy link
Contributor

Choose a reason for hiding this comment

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

We are not referencing added variable anywhere? Can we remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ArafatKhan2198 though we are not using, but we are using ternary operator here and without assigning to a variable, CI complains for findbugs.

@JsonInclude(JsonInclude.Include.NON_EMPTY)
private List<RepeatedOmKeyInfo> repeatedOmKeyInfoList;

@JsonProperty("deleteddirinfo")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@JsonProperty("deleteddirinfo")
@JsonProperty("deletedDirInfo")

Apply CamelCase here

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

private List<KeyEntityInfo> fsoKeyInfoList;

/** List of all deleted and repeatedly deleted keys. */
@JsonProperty("deletedkeyinfo")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@JsonProperty("deletedkeyinfo")
@JsonProperty("deletedKeyInfo")

Apply CamelCase here

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

/**
* HTTP Response wrapped for keys insights.
*/
public class KeyInsightInfoResp {
Copy link
Contributor

Choose a reason for hiding this comment

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

To make things consistent with naming conventions, it's probably a good idea to rename the response class. Instead of the current name, it would be better to call it either KeyInsightInfoResponse or KeyInsightResponse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

reconOMMetadataManager.getOpenKeyTable(getBucketLayout())
.get("/sampleVol/bucketOne/key_one");
Assertions.assertEquals("key_one", omKeyInfo1.getKeyName());
Response openKeyInfoResp = omdbInsightEndpoint.getOpenKeyInfo(-1, "");
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, the testGetOpenKeyInfo() method is only testing a single case. It would be good to test multiple cases such as when there are multiple open keys in the open key table or when the limit and prevKeyPrefix query parameters are used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ArafatKhan2198 - thanks for review. I have added few more test cases.

reconContainerManager
.updateContainerState(ContainerID.valueOf(1),
HddsProtos.LifeCycleEvent.CLEANUP);
containerIDs = containerStateManager
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 necessary to follow the entire Container lifecycle states from FINALIZE -> CLOSE -> DELETE -> DELETING -> CLEANUP -> DELETED and can't we just mark them as deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we need to follow the lifecycle, else it throws Invalid state.

.get("/sampleVol/bucketOne/key_one");
Assertions.assertEquals("key_one",
repeatedOmKeyInfo1.getOmKeyInfoList().get(0).getKeyName());
Response deletedKeyInfo = omdbInsightEndpoint.getDeletedKeyInfo(-1, "");
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, the testGetDeletedKeyInfo() method is only testing a single case. It would be good to test multiple cases such as when there are multiple open keys in the open key table or when the limit and prevKeyPrefix query parameters are used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ArafatKhan2198 - thanks for review. I have added few more test cases.

assertEquals(1, deletedSCMContainers.size());

Response deletedContainerKeysInfo =
omdbInsightEndpoint.getDeletedContainerKeysInfo(-1, "");
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, the testGetDeletedContainerKeysInfo() method is only testing a single case. It would be good to test multiple cases such as when there are multiple open keys in the open key table or when the limit and prevKeyPrefix query parameters are used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ArafatKhan2198 - thanks for review. I have added few more test cases.

private final ReconContainerManager containerManager;

@Inject
public OMDBInsightEndpoint(OzoneStorageContainerManager reconSCM,

Choose a reason for hiding this comment

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

LGTM .Just minor comment in case this endpoint class you want to name something with Keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Class names can be used to group the set of APIs belonged to specific feature, so I feel current name make sense here. So in future if we have more APIs related to keys, we should rather adding to same class, we can create one more class with meaningful name having APIs following the naming conventions.

@krishnaasawa1
Copy link

For API documentation update you can open a separate task & update accordingly post merge.

@devmadhuu
Copy link
Contributor Author

For API documentation update you can open a separate task & update accordingly post merge.

Sure. Here it is created . HDDS-8481

Copy link
Contributor

@sumitagrawl sumitagrawl left a comment

Choose a reason for hiding this comment

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

@devmadhuu Thanks for working over this, have few comments

* containers in DELETED state in SCM. */
@GET
@Path("/mismatch/keys")
public Response getDeletedContainerKeysInfo(
Copy link
Contributor

Choose a reason for hiding this comment

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

Unable to get use case where getting keys only for deleted container from SCM. I think we should return all keys which is not present in SCM (including deleted key) which may have data loss.
Additionally need see if gap between SCM and DN if DN do not have data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unable to get use case where getting keys only for deleted container from SCM. I think we should return all keys which is not present in SCM (including deleted key) which may have data loss. Additionally need see if gap between SCM and DN if DN do not have data.

As discussed, Please conclude on the use case.

if (seekKeyValue == null ||
(StringUtils.isNotBlank(prevKeyPrefix) &&
!seekKeyValue.getKey().equals(prevKeyPrefix))) {
return Response.ok(openKeyInsightInfo).build();
Copy link
Contributor

Choose a reason for hiding this comment

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

Here iterating over Legacy and FSO, if Legacy records not found, it will come out from loop without going to FSO. Please recheck this logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here iterating over Legacy and FSO, if Legacy records not found, it will come out from loop without going to FSO. Please recheck this logic.

Thanks for pointing out Sumit, This is fixed.

deletedKeyAndDirInsightInfo = new KeyInsightInfoResponse();
getPendingForDeletionKeyInfo(limit, prevKeyPrefix,
deletedKeyAndDirInsightInfo);
getPendingForDeletionDirInfo(limit, prevKeyPrefix,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do both have separate limit or combined limit for keyInfo and DirInfo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do both have separate limit or combined limit for keyInfo and DirInfo?

As of now, its combined limit because limit is done by keeping UI page in mind and API gives based on both keyInfo and DirInfo

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. combine limit means? limit = sizeof(keyInfoList) + sizeof(dirInfoList) ? currently its applied separately
  2. prevKeyPrefix is applied for both keyInfoList and dirInfoList, one will fail and other will success

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. combine limit means? limit = sizeof(keyInfoList) + sizeof(dirInfoList) ? currently its applied separately
  2. prevKeyPrefix is applied for both keyInfoList and dirInfoList, one will fail and other will success

@sumitagrawl - As discussed, created 2 separate APIs for pendingDeleteKeys and pendingDirs. Pls review.

}
updateReplicatedAndUnReplicatedTotal(deletedKeyAndDirInsightInfo,
repeatedOmKeyInfo);
repeatedOmKeyInfoList.add(repeatedOmKeyInfo);
Copy link
Contributor

Choose a reason for hiding this comment

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

how UI will know about previous key? as this is not given in output

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how UI will know about previous key? as this is not given in output

This is fixed, returning the last Key in response.

@DefaultValue(DEFAULT_FETCH_COUNT) @QueryParam(RECON_QUERY_LIMIT)
int limit,
@DefaultValue(StringUtils.EMPTY) @QueryParam(RECON_QUERY_PREVKEY)
String prevKeyPrefix) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since query to different information, key Info and dir Info, do need have separate information for query separately OR it will be in sequence after finish dir and then query key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since query to different information, key Info and dir Info, do need have separate information for query separately OR it will be in sequence after finish dir and then query key?

It will be in sequence.

@devmadhuu
Copy link
Contributor Author

devmadhuu commented May 11, 2023

As discussed, we'll be adding the total number of keys (open and deletePending) and amount of data mapped to those keys in a different PR to limit the code size in this PR, as this info will be added with the help of async task.

Below 3 PRs being raised for handling above: HDDS-8586, HDDS-8585, HDDS-8627

Copy link
Contributor

@sumitagrawl sumitagrawl left a comment

Choose a reason for hiding this comment

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

@devmadhuu Overall looks good to me, with minor comment for updating output sample with new changes.

* ]
* }
* ],
* "status": "OK"
Copy link
Contributor

Choose a reason for hiding this comment

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

Update the output sample with changes adding lastKey

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update the output sample with changes adding lastKey

Updated the sample response. Pls review.

Copy link
Contributor

@sumitagrawl sumitagrawl left a comment

Choose a reason for hiding this comment

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

@devmadhuu LGTM +1

@sumitagrawl sumitagrawl merged commit 5a6865f into apache:master May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants