You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "devabhishekpal (via GitHub)" <gi...@apache.org> on 2023/08/06 07:41:03 UTC
[GitHub] [ozone] devabhishekpal opened a new pull request, #5154: HDDS-9124. Handling network requests when context changes
devabhishekpal opened a new pull request, #5154:
URL: https://github.com/apache/ozone/pull/5154
## What changes were proposed in this pull request?
With the current code for Recon UI, we are not handling scenarios where network requests should be cancelled when user is shifting pages/context.
This causes unnecessary requests to be made.
For example if the user opens the Recon UI and selects a view, then the user changes that view immediately, the request for the previous view will still go through, along with the new request for the changed view.
This is more noticeable in a slow network, where requests might be stuck in a pending state, and during that time if we switch the view the previous request will still be sent, along with the new request.
The scenario is attached in the video recording, where a slow 3G network is being emulated.
If we are clicking on DiskUsage view it sends a request to:
`.../du?path=/&files=true`
Now if we click on the Containers view the second request is being sent for:
`.../unhealthy`
At this point we will notice both the requests to be in a pending state. And later both the requests are resolved.
Ideally we should be cancelling the DiskUsage request the moment we shift to the container view.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-9124
## How was this patch tested?
The patch was tested manually.
https://github.com/apache/ozone/assets/43001336/e5b2bef5-443d-4cd1-8aa5-ab5fc921c6dd
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devabhishekpal commented on a diff in pull request #5154: HDDS-9124. Handling network requests when context changes
Posted by "devabhishekpal (via GitHub)" <gi...@apache.org>.
devabhishekpal commented on code in PR #5154:
URL: https://github.com/apache/ozone/pull/5154#discussion_r1301852017
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/views/insights/insights.tsx:
##########
@@ -262,6 +267,10 @@ export class Insights extends React.Component<Record<string, object>, IInsightsS
});
}
+ componentWillUnmount(): void {
+ cancelInsightSignal && cancelInsightSignal.abort();
Review Comment:
Please refer to https://github.com/apache/ozone/pull/5154#discussion_r1301557777 for the same
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl merged pull request #5154: HDDS-9124. Handling network requests when context changes
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl merged PR #5154:
URL: https://github.com/apache/ozone/pull/5154
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on a diff in pull request #5154: HDDS-9124. Handling network requests when context changes
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #5154:
URL: https://github.com/apache/ozone/pull/5154#discussion_r1285409313
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/views/diskUsage/diskUsage.tsx:
##########
@@ -221,6 +241,13 @@ export class DiskUsage extends React.Component<Record<string, object>, IDUState>
this.updatePieChart('/', DEFAULT_DISPLAY_LIMIT);
}
+ componentWillUnmount(): void {
Review Comment:
Thanks @devabhishekpal for working on this patch. So this "componentWillUnmount" is a call back method for every view when a view gets changed ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on pull request #5154: HDDS-9124. Handling network requests when context changes
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on PR #5154:
URL: https://github.com/apache/ozone/pull/5154#issuecomment-1688559591
Thanks @devabhishekpal for handling the comments. Now LGTM +1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devabhishekpal commented on a diff in pull request #5154: HDDS-9124. Handling network requests when context changes
Posted by "devabhishekpal (via GitHub)" <gi...@apache.org>.
devabhishekpal commented on code in PR #5154:
URL: https://github.com/apache/ozone/pull/5154#discussion_r1301557777
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/views/insights/om/om.tsx:
##########
@@ -448,14 +454,34 @@ export class Om extends React.Component<Record<string, object>, IOmdbInsightsSta
this.fetchMismatchContainers(this.state.DEFAULT_LIMIT, this.state.prevKeyMismatch, this.state.mismatchMissingState);
};
+ componentWillUnmount(): void {
+ cancelMismatchedEndpointSignal && cancelMismatchedEndpointSignal.abort();
Review Comment:
So for abort() it doesn't directly pass the message, in the axios wrapper where I cancelling the requests, in that only I added the common message that request changed.
Check [common.tsx:38](https://github.com/apache/ozone/blob/293fd76b63a663269633818eba771f3cc6c77911/hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/utils/common.tsx#L38)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devabhishekpal commented on a diff in pull request #5154: HDDS-9124. Handling network requests when context changes
Posted by "devabhishekpal (via GitHub)" <gi...@apache.org>.
devabhishekpal commented on code in PR #5154:
URL: https://github.com/apache/ozone/pull/5154#discussion_r1285615107
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/views/datanodes/datanodes.tsx:
##########
@@ -339,7 +341,14 @@ export class Datanodes extends React.Component<Record<string, object>, IDatanode
loading: true,
selectedColumns: this._getSelectedColumns(prevState.selectedColumns)
}));
- axios.get('/api/v1/datanodes').then(response => {
+
+ if (typeof cancelToken != typeof undefined){
+ //Some existing request might be there so we will cancel that before sending new request
+ cancelToken.cancel("Cancelling old datanode request");
Review Comment:
This is handling cases, when say autoReload happens, it will call _loadData(), but the component is not unmounted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on a diff in pull request #5154: HDDS-9124. Handling network requests when context changes
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #5154:
URL: https://github.com/apache/ozone/pull/5154#discussion_r1297952071
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/views/insights/insights.tsx:
##########
@@ -262,6 +267,10 @@ export class Insights extends React.Component<Record<string, object>, IInsightsS
});
}
+ componentWillUnmount(): void {
+ cancelInsightSignal && cancelInsightSignal.abort();
Review Comment:
Are we not adding any message here in abort function like we did in heatmap.tsx ?
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/views/insights/om/om.tsx:
##########
@@ -448,14 +454,34 @@ export class Om extends React.Component<Record<string, object>, IOmdbInsightsSta
this.fetchMismatchContainers(this.state.DEFAULT_LIMIT, this.state.prevKeyMismatch, this.state.mismatchMissingState);
};
+ componentWillUnmount(): void {
+ cancelMismatchedEndpointSignal && cancelMismatchedEndpointSignal.abort();
Review Comment:
Are we not adding any message here in all abort functions like we did in heatmap.tsx ? Pls check all Unmount functions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smitajoshi12 commented on pull request #5154: HDDS-9124. Handling network requests when context changes
Posted by "smitajoshi12 (via GitHub)" <gi...@apache.org>.
smitajoshi12 commented on PR #5154:
URL: https://github.com/apache/ozone/pull/5154#issuecomment-1669474737
1) First Use Axios signal instead of cancelToken as it is deprecated.
2) do testing with Cluster data and attach video.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on a diff in pull request #5154: HDDS-9124. Handling network requests when context changes
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #5154:
URL: https://github.com/apache/ozone/pull/5154#discussion_r1286996019
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/views/datanodes/datanodes.tsx:
##########
@@ -339,7 +341,14 @@ export class Datanodes extends React.Component<Record<string, object>, IDatanode
loading: true,
selectedColumns: this._getSelectedColumns(prevState.selectedColumns)
}));
- axios.get('/api/v1/datanodes').then(response => {
+
+ if (typeof cancelToken != typeof undefined){
+ //Some existing request might be there so we will cancel that before sending new request
+ cancelToken.cancel("Cancelling old datanode request");
Review Comment:
As discussed, pls refactor it to reduce duplicity of code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on a diff in pull request #5154: HDDS-9124. Handling network requests when context changes
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #5154:
URL: https://github.com/apache/ozone/pull/5154#discussion_r1285491211
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/views/datanodes/datanodes.tsx:
##########
@@ -339,7 +341,14 @@ export class Datanodes extends React.Component<Record<string, object>, IDatanode
loading: true,
selectedColumns: this._getSelectedColumns(prevState.selectedColumns)
}));
- axios.get('/api/v1/datanodes').then(response => {
+
+ if (typeof cancelToken != typeof undefined){
+ //Some existing request might be there so we will cancel that before sending new request
+ cancelToken.cancel("Cancelling old datanode request");
Review Comment:
If "componentWillUnmount" will cancel the request on changing the view, then what is the possibility of older request still be there ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devabhishekpal commented on a diff in pull request #5154: HDDS-9124. Handling network requests when context changes
Posted by "devabhishekpal (via GitHub)" <gi...@apache.org>.
devabhishekpal commented on code in PR #5154:
URL: https://github.com/apache/ozone/pull/5154#discussion_r1285732986
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/views/datanodes/datanodes.tsx:
##########
@@ -339,7 +341,14 @@ export class Datanodes extends React.Component<Record<string, object>, IDatanode
loading: true,
selectedColumns: this._getSelectedColumns(prevState.selectedColumns)
}));
- axios.get('/api/v1/datanodes').then(response => {
+
+ if (typeof cancelToken != typeof undefined){
+ //Some existing request might be there so we will cancel that before sending new request
+ cancelToken.cancel("Cancelling old datanode request");
Review Comment:
We cannot explicitly call componentWillUnmount. It is auto triggered before the component is just about to be removed from DOM. Take it as the equivalent of say stop() for a thread, only we cannot explicitly do this.componentWillUnmount() to call it. So I had request cancellation handled in _loadData() separately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devabhishekpal commented on a diff in pull request #5154: HDDS-9124. Handling network requests when context changes
Posted by "devabhishekpal (via GitHub)" <gi...@apache.org>.
devabhishekpal commented on code in PR #5154:
URL: https://github.com/apache/ozone/pull/5154#discussion_r1285732986
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/views/datanodes/datanodes.tsx:
##########
@@ -339,7 +341,14 @@ export class Datanodes extends React.Component<Record<string, object>, IDatanode
loading: true,
selectedColumns: this._getSelectedColumns(prevState.selectedColumns)
}));
- axios.get('/api/v1/datanodes').then(response => {
+
+ if (typeof cancelToken != typeof undefined){
+ //Some existing request might be there so we will cancel that before sending new request
+ cancelToken.cancel("Cancelling old datanode request");
Review Comment:
We cannot explicitly call componentWillUnmount. It is auto triggered before the component is just about to be removed from DOM. Take it as the equivalent of say stop() for a thread, only we cannot explicitly do this.componentWillUnmount() to call it. So I had it handled in _loadData().
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on a diff in pull request #5154: HDDS-9124. Handling network requests when context changes
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #5154:
URL: https://github.com/apache/ozone/pull/5154#discussion_r1301569707
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/views/insights/om/om.tsx:
##########
@@ -448,14 +454,34 @@ export class Om extends React.Component<Record<string, object>, IOmdbInsightsSta
this.fetchMismatchContainers(this.state.DEFAULT_LIMIT, this.state.prevKeyMismatch, this.state.mismatchMissingState);
};
+ componentWillUnmount(): void {
+ cancelMismatchedEndpointSignal && cancelMismatchedEndpointSignal.abort();
Review Comment:
I was mentioning about below line.
[heatmap.tsx:211](https://github.com/apache/ozone/blob/293fd76b63a663269633818eba771f3cc6c77911/hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/views/heatMap/heatmap.tsx#L211)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devabhishekpal commented on a diff in pull request #5154: HDDS-9124. Handling network requests when context changes
Posted by "devabhishekpal (via GitHub)" <gi...@apache.org>.
devabhishekpal commented on code in PR #5154:
URL: https://github.com/apache/ozone/pull/5154#discussion_r1296069336
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/views/datanodes/datanodes.tsx:
##########
@@ -339,7 +341,14 @@ export class Datanodes extends React.Component<Record<string, object>, IDatanode
loading: true,
selectedColumns: this._getSelectedColumns(prevState.selectedColumns)
}));
- axios.get('/api/v1/datanodes').then(response => {
+
+ if (typeof cancelToken != typeof undefined){
+ //Some existing request might be there so we will cancel that before sending new request
+ cancelToken.cancel("Cancelling old datanode request");
Review Comment:
Refactored abort requests in commit: [bded747](https://github.com/apache/ozone/pull/5154/commits/bded747b423c591f68b4ff8ffda1bf877c0395f7)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devabhishekpal commented on pull request #5154: HDDS-9124. Handling network requests when context changes
Posted by "devabhishekpal (via GitHub)" <gi...@apache.org>.
devabhishekpal commented on PR #5154:
URL: https://github.com/apache/ozone/pull/5154#issuecomment-1682241599
Updated video for patch:
https://github.com/apache/ozone/assets/43001336/722ef80e-32fa-48b4-81c0-8d77c38cf659
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on a diff in pull request #5154: HDDS-9124. Handling network requests when context changes
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #5154:
URL: https://github.com/apache/ozone/pull/5154#discussion_r1285703237
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/views/datanodes/datanodes.tsx:
##########
@@ -339,7 +341,14 @@ export class Datanodes extends React.Component<Record<string, object>, IDatanode
loading: true,
selectedColumns: this._getSelectedColumns(prevState.selectedColumns)
}));
- axios.get('/api/v1/datanodes').then(response => {
+
+ if (typeof cancelToken != typeof undefined){
+ //Some existing request might be there so we will cancel that before sending new request
+ cancelToken.cancel("Cancelling old datanode request");
Review Comment:
> This is handling cases, when say autoReload happens, it will call _loadData(), but the component is not unmounted.
So is it possible to override _loadData() and call "componentWillUnmount" explicitly inside it ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devabhishekpal commented on a diff in pull request #5154: HDDS-9124. Handling network requests when context changes
Posted by "devabhishekpal (via GitHub)" <gi...@apache.org>.
devabhishekpal commented on code in PR #5154:
URL: https://github.com/apache/ozone/pull/5154#discussion_r1285597702
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/views/diskUsage/diskUsage.tsx:
##########
@@ -221,6 +241,13 @@ export class DiskUsage extends React.Component<Record<string, object>, IDUState>
this.updatePieChart('/', DEFAULT_DISPLAY_LIMIT);
}
+ componentWillUnmount(): void {
Review Comment:
componentWillUnmount() is basically a lifecycle method, which is called whenever a component is going to get removed from the DOM, so essentially, in this case, when we are switching the views.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org