Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add totalSize and totalCount values to VFS stats #66

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mahsashadi
Copy link
Contributor

Due to questions mentioned here, and also solutions provided here and here, we might need totalCount and totalSize in stats of directories.

@mahsashadi
Copy link
Contributor Author

Just I cant not fix the unit-test of stats.
Although stats work properly, there is some problem with totalSize which causes failing of the test.

__tests__/adapters/vfs/system.js Outdated Show resolved Hide resolved
__tests__/adapters/vfs/system.js Outdated Show resolved Hide resolved
@mahsashadi mahsashadi changed the title Add totalSize and totalCount values to stats Add totalSize and totalCount values to VFS stats Aug 3, 2022
__tests__/adapters/vfs/system.js Outdated Show resolved Hide resolved
__tests__/adapters/vfs/system.js Outdated Show resolved Hide resolved
filename,
stat
});
const createStat = async stat => {
Copy link
Member

Choose a reason for hiding this comment

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

This method of figuring out the total size of a directory seems very inefficient, especially when there's a lot of files.

Maybe it would be better looking into spawning a system process to do this in a native way ? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I searched, du can be used liked this:

const child = spawn('sh', ['-c', 'du -s ${realPath} | cut -f1']);

But the problem is that du seems to have considerable delay, especially for large directories.

Copy link
Member

Choose a reason for hiding this comment

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

especially for large directories.

But I would assume it's faster than doing it with node, right?

Copy link
Member

Choose a reason for hiding this comment

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

Also I'm not 100% sure, but I assume that this wouldn't be cross-compatible with users hoping to run OS.js locally on Windows. IMHO native system processes shouldn't be included without a fallback for Windows

Copy link
Member

Choose a reason for hiding this comment

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

There's a du npm package that handles Windows etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In new commit du package is used for totalSize. @andersevenrud

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am thinking about our own vfs system adapter, which is a cloud object storage.
Since totalSize is not provided in APIs, I have to iterate through files which as you said, gets lots of time. Is there any other solution?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I can't think of a good solution to that, especially if there's pagination involved :/

Copy link
Member

@ajmeese7 ajmeese7 left a comment

Choose a reason for hiding this comment

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

Obviously I'm going to defer to @andersevenrud since he is the SME, but this looks better and simplified to me. Good work!

@mahsashadi
Copy link
Contributor Author

Hi again @andersevenrud
Any more changes needed to do in this PR?

@andersevenrud
Copy link
Member

@mahsashadi I've just been too busy to keep up with all of the PRs :(

I've made a notification in my calendar for this weekend so I can test and approve this.

@mahsashadi
Copy link
Contributor Author

Hi again after long time.
It will be great if we can approve these changes.
Thanks.
@andersevenrud @ajmeese7

Copy link
Member

@andersevenrud andersevenrud left a comment

Choose a reason for hiding this comment

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

It's been a long time indeed @mahsashadi. If only I had more time to follow up on things 😅

This looks good to me, but there's two very minor things:

  1. Could you add a configuration option for this (and keep it on by default) ?
  2. Resolve the conflict so it can be merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants