HDDS-13188. Track total pending deletion size for FSO directories in OM via Recon #9252
Conversation
devmadhuu
left a comment
devmadhuu
left a comment
There was a problem hiding this comment.
@priyeshkaratha thanks for the patch, pls check my comment.
| * } | ||
| */ | ||
| @GET | ||
| @Path("/deletePending/dirs/size-summary") |
There was a problem hiding this comment.
How this API is different from existing one here ?
There was a problem hiding this comment.
@devmadhuu , this "/deletePending/dirs/size-summary" is a new API, returns calculated size and replicated size.
"/deletePending/dirs/summary" is a existing API, returns count from DB directly without calculation.
Do you think we should merge them into one, or keep it separately?
There was a problem hiding this comment.
I can see that existing API and new API both are iterating deletedDirTable, Only difference is that new API is using size computation which is pre-computed in NSSummary and existing API is also using same NSSummary precomputed sizes ? Then what exactly is the difference, and why we need a separate API if existing API will serve the purpose ? May be we can use existing API with handling of limit param which is 1000 by default.
There was a problem hiding this comment.
Anyway we are using this api inside recon only. So I think instead of exposing as API, we can use calculateTotalPendingDeletedDirSizes method as public method. So that we can use it in newly added recon endpoint. What is your thoughts? @ChenSammi @devmadhuu
There was a problem hiding this comment.
I still not able to understand, why we need that new API code ? Existing API, very minor changes needed related to limit. Can you explain ?
There was a problem hiding this comment.
It's a summary API, so I guess limit doesn't apply. Ideally, we should add the new function in "/deletePending/dirs/summary" too, as the pending deletion key does, so all the APIs' behavior is consistent.
* Example response:
* {
* "totalDeletedKeys": 8,
* "totalReplicatedDataSize": 90000,
* "totalUnreplicatedDataSize": 30000
* }
*/
@GET
@Path("/deletePending/summary")
public Response getDeletedKeySummary() {
The difference is for directory, it iterate the omMetadataManager.getDeletedDirTable() table to calculate all the sizes, for key, it does this
// Fetch the necessary metrics for deleted keys
Long replicatedSizeDeleted = getValueFromId(reconGlobalStatsManager.getGlobalStatsValue(
OmTableInsightTask.getReplicatedSizeKeyFromTable(DELETED_TABLE)));
Long unreplicatedSizeDeleted = getValueFromId(reconGlobalStatsManager.getGlobalStatsValue(
OmTableInsightTask.getUnReplicatedSizeKeyFromTable(DELETED_TABLE)));
Long deletedKeyCount = getValueFromId(reconGlobalStatsManager.getGlobalStatsValue(
OmTableInsightTask.getTableCountKeyFromTable(DELETED_TABLE)));
I'm not very familiar with Recon code. So does above code just read the data from somewhere and doesn't require any further calculation?
My concern regarding how we use "/deletePending/summary" and "/deletePending/dirs/summary" in Recon UI, if pending deletion directory size calculation is expensive, and in Recon UI, where only totalDeletedDirectories is required, if we calculate the size in each API call, the data is not used and wasted, in meanwhile also add the latency to this API call. I'm fine to merge the two API into one if that's not a problem.
There was a problem hiding this comment.
I think we can close this PR. Instead of maintaining two separate methods, we can simplify the logic by passing limit = -1 when we want to retrieve all results. Inside the loop, we can add a check like if (limit > 0 && resultSize == limit) to break when the limit is reached . This way, the same method can handle both limited and complete results without additional code duplication.
There was a problem hiding this comment.
@ChenSammi This PR is adding an API for deletePending directories size summary. These all existing APIs /deletePending (for pending delete keys), /deletePending/dirs (for deletePending directories) and /deletePending/dir/summary are for different purpose respectively. As far as I understand, currently /deletePending/dir/summary API is not being used in Recon UI anywhere and just returning count pre-computed from OmTableInsightTask and not on the fly. And /deletePending/dirs API is iterating deletedDirTable in same way as this new API /deletePending/dirs/size-summary in this PR is iterating the table. If you see the difference in their code, there is not much difference and rather existing API is more sophisticated and supports pagination as well. Existing API and new API in this PR both are using the size values (replicated , unreplicated) from pre-computed data from NSSummary and will provide similar performance and same data. @priyeshkaratha can test and confirm. It is just that , If we can handle limit param with -1 (all values),, then no need of new API in this PR, because new API in this PR also iterating all keys (records) from deletedDirTable.
| totalReplicatedDataSize += sizeInfo.getRight(); | ||
| } | ||
| } | ||
| } catch (IOException ex) { |
There was a problem hiding this comment.
Please log a message for this ex to know why it fails.
|
@devmadhuu @ChenSammi I was able to use existing method in Recon. So closing this PR |
5 participants
What changes were proposed in this pull request?
The PR introduces a
GET /api/v1/keys/deletePending/dirs/size-summaryendpoint to includetotalUnreplicatedDataSizeandtotalReplicatedDataSizefields for tracking the total pending deletion size for FSO directories in OM.totalReplicatedDataSize: Total size of directories pending deletion with expected replicated size.totalUnreplicatedDataSize: Total raw size of directories pending deletion without replicated size.What is the link to the Apache JIRA
HDDS-13188
How was this patch tested?
CI is green.
Test details can be found here.