Skip to content

Conversation

@smitajoshi12
Copy link
Contributor

@smitajoshi12 smitajoshi12 commented Jul 1, 2024

What changes were proposed in this pull request?

To avoid null conditions on Disk Usage page

Please describe your PR in detail:

When Recon was intializing on Disk Usage page we were getting failures as values are not mapped properly so to avoid such conditions added checks.

API Response :

api/v1/namespace/du?path=/&files=true&sortSubPaths=true

{ "status": "INITIALIZING", "path": null, "size": 0, "sizeWithReplica": -1, "subPathCount": 0, "subPaths": [], "sizeDirectKey": -1 }


api/v1/namespace/summary?path=null

{ "path": "", "type": "UNKNOWN", "countStats": null, "objectInfo": null, "status": "INITIALIZING" }


/api/v1/namespace/quota?path=null

{"allowed":0,"used":0,"status":"INITIALIZING"}

What is the link to the Apache JIRA

(https://issues.apache.org/jira/browse/HDDS-11023)

How was this patch tested?

Manually

Before this PR

image

With this Patch

image

@smitajoshi12
Copy link
Contributor Author

@devabhishekpal @dombizita @devmadhuu @ArafatKhan2198

Can you review this pr

Copy link
Contributor

@ArafatKhan2198 ArafatKhan2198 left a comment

Choose a reason for hiding this comment

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

Thanks for the patch @smitajoshi12
Some minor comments

smitajoshi12

This comment was marked as resolved.

@dombizita dombizita changed the title HDDS-11023. Recon Disk Usage null conditions not handled properly for… HDDS-11023. Recon Disk Usage null conditions not handled properly for null response Jul 11, 2024
@dombizita dombizita self-requested a review July 11, 2024 09:27
Copy link
Contributor

@devabhishekpal devabhishekpal left a comment

Choose a reason for hiding this comment

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

Thanks @smitajoshi12 , the patch looks good to me now.

Please do take a look at @ArafatKhan2198's comment for replica=false and raise the JIRA.

@smitajoshi12
Copy link
Contributor Author

smitajoshi12 commented Jul 12, 2024

Thanks @smitajoshi12 , the patch looks good to me now.

Please do take a look at @ArafatKhan2198's comment for replica=false and raise the JIRA.

@devabhishekpal
It is already raised. [(https://issues.apache.org/jira/browse/HDDS-11140)]

@ArafatKhan2198
Copy link
Contributor

Thanks for working on this patch, @smitajoshi12 I appreciate you catching this issue and implementing the necessary changes. However, I believe we can present the information displayed to the user about Namespace initialization in a much clearer way, as the Namespace can be in initialization mode for two specific reasons:

  1. The metadata is too large and is taking time to initialize.
  2. The OM snapshot has not yet been fetched from the OM.

It's important for the user to know that there is no problem with the data; rather, there is just a delay in building the NSSummary. Instead of having the user click the "Show Metadata for Current Path" button, we should display a pop-up message. This message should not be an alert but a simple notification indicating that the state is initializing and that they need to wait.

Additionally, the error message shown in the screenshot can be improved to something more user-friendly like:

"Metadata Initialization: The metadata is currently initializing. Please wait a moment and try again later.

image

@smitajoshi12
Copy link
Contributor Author

Thanks for working on this patch, @smitajoshi12 I appreciate you catching this issue and implementing the necessary changes. However, I believe we can present the information displayed to the user about Namespace initialization in a much clearer way, as the Namespace can be in initialization mode for two specific reasons:

  1. The metadata is too large and is taking time to initialize.
  2. The OM snapshot has not yet been fetched from the OM.

It's important for the user to know that there is no problem with the data; rather, there is just a delay in building the NSSummary. Instead of having the user click the "Show Metadata for Current Path" button, we should display a pop-up message. This message should not be an alert but a simple notification indicating that the state is initializing and that they need to wait.

Additionally, the error message shown in the screenshot can be improved to something more user-friendly like:

"Metadata Initialization: The metadata is currently initializing. Please wait a moment and try again later.

image

@ArafatKhan2198
Completed Changes suggested by you attached screenshot with Info Notification

Copy link
Contributor

@devabhishekpal devabhishekpal left a comment

Choose a reason for hiding this comment

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

Added minor check changes to use optional chaining instead

… null response Review Comment for Optional Chain
Copy link
Contributor

@ArafatKhan2198 ArafatKhan2198 left a comment

Choose a reason for hiding this comment

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

Thanks @smitajoshi12 for the patch overall the changes look good.
Just a few more minor comments

@ArafatKhan2198
Copy link
Contributor

@devabhishekpal Can you please go through your comments and resolve them if they have been fixed in the patch.

Copy link
Contributor

@devabhishekpal devabhishekpal left a comment

Choose a reason for hiding this comment

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

Thanks @smitajoshi12 for the changes.
Please do take a look at @ArafatKhan2198's comments.
Apart from that it looks good to me, +1

@smitajoshi12
Copy link
Contributor Author

smitajoshi12 commented Jul 25, 2024

@ArafatKhan2198
Completed all changes can you merge this pr.

@ArafatKhan2198
Copy link
Contributor

@smitajoshi12 Most of the changes look good, but I noticed something odd while testing this patch on a cluster. When there is supposed to be no data in the cluster except for a volume named S3V, clicking the "Show Metadata" button should display the NSSummary for the root ("/") path. However, we're encountering an error instead. I'm not sure if this issue was introduced by this patch, but could you please look into it as well?

image

Copy link
Contributor

@devmadhuu devmadhuu left a comment

Choose a reason for hiding this comment

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

Thanks @smitajoshi12 for improving the patch. Changes LGTM +1. Thanks @ArafatKhan2198 @dombizita @devabhishekpal for review.

@smitajoshi12
Copy link
Contributor Author

smitajoshi12 commented Jul 26, 2024

@smitajoshi12 Most of the changes look good, but I noticed something odd while testing this patch on a cluster. When there is supposed to be no data in the cluster except for a volume named S3V, clicking the "Show Metadata" button should display the NSSummary for the root ("/") path. However, we're encountering an error instead. I'm not sure if this issue was introduced by this patch, but could you please look into it as well?

image

@ArafatKhan2198 @devpalabhishek
Optional Chaining not working summaryResponse.objectInfo?.replicationConfig?.replicationFactor !== -1 Instead -1 need to check conditions for undefined also

{
"path": "",
"type": "ROOT",
"countStats": {
"numVolume": 1,
"numBucket": 0,
"numDir": 0,
"numKey": 0
},
"objectInfo": {
"metadata": null,
"name": null,
"quotaInBytes": 0,
"quotaInNamespace": 0,
"usedNamespace": 0,
"creationTime": 0,
"modificationTime": 0,
"acls": null
},
"status": "OK"
}

@smitajoshi12
Copy link
Contributor Author

@smitajoshi12 Most of the changes look good, but I noticed something odd while testing this patch on a cluster. When there is supposed to be no data in the cluster except for a volume named S3V, clicking the "Show Metadata" button should display the NSSummary for the root ("/") path. However, we're encountering an error instead. I'm not sure if this issue was introduced by this patch, but could you please look into it as well?
image

@ArafatKhan2198 @devpalabhishek Optional Chaining not working summaryResponse.objectInfo?.replicationConfig?.replicationFactor !== -1 Instead -1 need to check conditions for undefined also

{ "path": "", "type": "ROOT", "countStats": { "numVolume": 1, "numBucket": 0, "numDir": 0, "numKey": 0 }, "objectInfo": { "metadata": null, "name": null, "quotaInBytes": 0, "quotaInNamespace": 0, "usedNamespace": 0, "creationTime": 0, "modificationTime": 0, "acls": null }, "status": "OK" }

@ArafatKhan2198 Thanks for pointing out undefined condition. In Latest commit I. have handled both undefined and -1 condition from API response.

Copy link
Contributor

@devabhishekpal devabhishekpal left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @smitajoshi12.
Thanks for reviewing the change @ArafatKhan2198, @devmadhuu, @dombizita

@ArafatKhan2198 ArafatKhan2198 merged commit 82c6bf3 into apache:master Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants