Bug #15840

[Workbench2] Project page large file count hangs

Added by Eric Biagiotti 7 months ago. Updated 6 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Workbench2
Target version:
Start date:
11/19/2019
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-

Description

The project panel loads the entire collection file tree to calculate file count and total size. Uses the file_count and file_size_total collection attributes instead.


Subtasks

Task #15841: Review 15610-large-collection-timeoutResolvedPeter Amstutz

History

#1 Updated by Eric Biagiotti 7 months ago

Latest @ commit:c6314ca47008f1ffd0fe70fe025a57475b64e773 in the wb2 repo.

Now uses the file_count and file_size_total collection attributes. Also updated the collection service to not do any tree manipulation when using propfind for files.

#2 Updated by Eric Biagiotti 7 months ago

  • Target version changed from 2019-11-20 Sprint to 2019-12-04 Sprint

#3 Updated by Peter Amstutz 7 months ago

  • Target version deleted (2019-12-04 Sprint)

Functionally this seems to work fine but it took me some time to understand all the changes, this would be a good opportunity for some knowledge sharing.

collection-panel-files-actions.ts:28

I had a hard time understanding the purpose of each step in this method. Please add some comments.

export const loadCollectionFiles = (uuid: string) =>
    async (dispatch: Dispatch, getState: () => RootState, services: ServiceRepository) => {
        const files = await services.collectionService.files(uuid);
        const tree = createCollectionFilesTree(files);
        const sorted = sortFilesTree(tree);
        const mapped = mapTreeValues(services.collectionService.extendFileURL)(sorted);
        dispatch(collectionPanelFilesAction.SET_COLLECTION_FILES(mapped));
    };

Looking at the diff, where the old code had dispatch(resourcesDataActions.SET_FILES({ uuid, files })); but now SET_FILES (actually all of ~/store/resources-data) is gone. I think maybe the purpose was to cache the results from WebDAV, does that mean it is reloading the file listing in cases it wouldn't have done it before? (That's probably fine if it only does it when you click on the collection page but still worth noting).

I think these are the two changes that actually matter, everything else is just cleanup?

-                api.dispatch<any>(updateResourceData(resourceUuids));
-        const resource = getResourceData(props.uuid)(state.resourcesData);
-        return { fileSize: resource ? resource.fileSize : 0 };
+        const resource = getResource<CollectionResource>(props.uuid)(state.resources);
+        return { fileSize: resource ? resource.fileSizeTotal : 0 };

Otherwise LGTM!

#4 Updated by Eric Biagiotti 6 months ago

  • Target version set to 2019-12-04 Sprint

#5 Updated by Eric Biagiotti 6 months ago

Peter Amstutz wrote:

Functionally this seems to work fine but it took me some time to understand all the changes, this would be a good opportunity for some knowledge sharing.

This should have been separated into a few commits. The refactoring included here was mainly to remove the tree parsing/construction from the files function in collection-service.ts, which should really only parse and return the webdav results. Not critical for this branch though, and I can remove if you think that would be better.

collection-panel-files-actions.ts:28

I had a hard time understanding the purpose of each step in this method. Please add some comments.
[...]

This was a result of the refactor mentioned above, but I added a comment to try and explain it a bit more.

Looking at the diff, where the old code had dispatch(resourcesDataActions.SET_FILES({ uuid, files })); but now SET_FILES (actually all of ~/store/resources-data) is gone. I think maybe the purpose was to cache the results from WebDAV, does that mean it is reloading the file listing in cases it wouldn't have done it before? (That's probably fine if it only does it when you click on the collection page but still worth noting).

store/resource-data code (dispatch(resourcesDataActions.SET_FILES({ uuid, files }))) was only used to reduce the collection file list to file count and total file size when the project-panel data explorer was being populated (ProjectPanelMiddlewareService.requestItems). It doesn't do any caching and since we now get those two attributes from the collection object, I was able to remove it all.

Latest @ ef8f1518f7d4537b63a5b0201ea10413a2712c0d

- Fixes webdav propfind parsing for directories and adds a comment

#6 Updated by Peter Amstutz 6 months ago

Eric Biagiotti wrote:

Peter Amstutz wrote:

Functionally this seems to work fine but it took me some time to understand all the changes, this would be a good opportunity for some knowledge sharing.

This should have been separated into a few commits. The refactoring included here was mainly to remove the tree parsing/construction from the files function in collection-service.ts, which should really only parse and return the webdav results. Not critical for this branch though, and I can remove if you think that would be better.

collection-panel-files-actions.ts:28

I had a hard time understanding the purpose of each step in this method. Please add some comments.
[...]

This was a result of the refactor mentioned above, but I added a comment to try and explain it a bit more.

Looking at the diff, where the old code had dispatch(resourcesDataActions.SET_FILES({ uuid, files })); but now SET_FILES (actually all of ~/store/resources-data) is gone. I think maybe the purpose was to cache the results from WebDAV, does that mean it is reloading the file listing in cases it wouldn't have done it before? (That's probably fine if it only does it when you click on the collection page but still worth noting).

store/resource-data code (dispatch(resourcesDataActions.SET_FILES({ uuid, files }))) was only used to reduce the collection file list to file count and total file size when the project-panel data explorer was being populated (ProjectPanelMiddlewareService.requestItems). It doesn't do any caching and since we now get those two attributes from the collection object, I was able to remove it all.

Latest @ ef8f1518f7d4537b63a5b0201ea10413a2712c0d

- Fixes webdav propfind parsing for directories and adds a comment

This LGTM.

#7 Updated by Eric Biagiotti 6 months ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF