HDDS-13465. Adding endpoint at Recon to get consolidated storage distribution report#8995
Conversation
priyeshkaratha
force-pushed
the
HDDS-13465_Metrics_Aproach
branch
from
6e1c590 to
bb69fa6
Compare
priyeshkaratha
changed the title
|
@ArafatKhan2198 @devmadhuu @sumitagrawl @ChenSammi Please review the code changes. |
devmadhuu
left a comment
devmadhuu
left a comment
There was a problem hiding this comment.
Thanks @priyeshkaratha for the patch. Few comments
| try { | ||
| conn.disconnect(); | ||
| } catch (Exception ignored) { | ||
| // no-op |
There was a problem hiding this comment.
How this is being handled at UI ?
There was a problem hiding this comment.
In UI, it will always get value of 0 or the actual values. So errors will be logged at server side.
| // ignore read errors on error stream | ||
| } | ||
| log.warn("HTTP {} from {}. Error body: {}", code, conn.getURL(), err); | ||
| return ""; |
There was a problem hiding this comment.
Empty response is valid response. How UI is going to handle in case of IOException which is ignored after being caught ?
There was a problem hiding this comment.
parseMetrics method will handle empty string and it will return 0L as metrics.
|
There is still an open design doc in this area in #8907. We should reach consensus on that before merging changes like this. |
ArafatKhan2198
left a comment
ArafatKhan2198
left a comment
There was a problem hiding this comment.
Thanks for the work @priyeshkaratha
Some comments and questions.
priyeshkaratha
force-pushed
the
HDDS-13465_Metrics_Aproach
branch
2 times, most recently
from
cebf13d to
5e818fa
Compare
priyeshkaratha
force-pushed
the
HDDS-13465_Metrics_Aproach
branch
from
bee8c10 to
0e367cf
Compare
|
@ArafatKhan2198 @devmadhuu Thanks for the reviews. Handled all the review comments and rebased with latest feature branch. |
priyeshkaratha
force-pushed
the
HDDS-13465_Metrics_Aproach
branch
from
adf05bb to
e6c934e
Compare
devmadhuu
left a comment
devmadhuu
left a comment
There was a problem hiding this comment.
Thanks @priyeshkaratha , changes largely looks fine to me. However you need to still address and close design points raised by @errose28 before merging.
ArafatKhan2198
left a comment
ArafatKhan2198
left a comment
There was a problem hiding this comment.
LGTM !
Thanks for working on this.
5 participants
What changes were proposed in this pull request?
A new REST API endpoint
/storagedistributionhas been introduced to provide a summarized view of storage distribution across the cluster. This endpoint is designed to consolidate various storage-related insights, particularly focusing on used capacity, pending deletions, and overall data distribution across Ozone components.Key Functionalities
/storagedistributionOMDBInsightEndpoint:
SCMClient:
Datanode Storage Reports:
Here the pending deletion bytes are calculated and updated in the BlockDeletionService metrics, which are then exposed via JMX. This allows for direct access to these metrics by monitoring tools. The primary disadvantage of this approach is the introduction of a dependency on JMX. While this might be a minor concern in some deployments, it's worth noting the additional dependency. This method avoids the unnecessary data transfer to SCM and the associated processing overhead in DN while iterating and calculating pending deletion from the container data list that is present with the Storage Report approach.
What is the link to the Apache JIRA
HDDS-13465
How was this patch tested?
Currently tested by unit test cases by mocking SCM and DN data. Also tested manually using postman.