Skip to content

199 show space bytes#263

Merged
lmarini merged 27 commits into
developfrom
199-show-space-bytes
Oct 4, 2021
Merged

199 show space bytes#263
lmarini merged 27 commits into
developfrom
199-show-space-bytes

Conversation

@tcnichol
Copy link
Copy Markdown
Contributor

@tcnichol tcnichol commented Aug 9, 2021

Description

the statistics view for spaces now shows bytes per space.

Review Time Estimate

  • When possible

Types of changes

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have updated the CHANGELOG.md.
  • I have signed the CLA
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@tcnichol tcnichol linked an issue Aug 9, 2021 that may be closed by this pull request
@tcnichol tcnichol requested review from max-zilla and robkooper August 9, 2021 22:20
Copy link
Copy Markdown
Contributor

@max-zilla max-zilla left a comment

Choose a reason for hiding this comment

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

Left a comment about possibly optimizing the logic of the counting. If it has already been discussed and this is the best way, just let me know.

Comment thread app/controllers/Spaces.scala Outdated
files.get(ds_f) match {
case Some(file) => {
files.getBytes(file.id) match {
case Some((stream, name, filetype, bytes)) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My concern with this is that it could be very slow on large spaces with many files, getting every dataset and every file individually... can we use some of the work done before these calls:

        val datasetsInSpace = datasets.listSpace(size, id.toString(), user)
        val spaceBytes : Long = getBytesPerSpace(id, user.get)
        val spaceFiles : Integer = getFilesPerSpace(id, user.get)

...instead of calling datasets.listSpace again for example?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think this is now fixed by caching the spaceBytes as part of the Space object. When datasets are added or files added, the bytes increment, or decrement if removed. So there won't be these calls anymore.

Comment thread app/controllers/Spaces.scala Outdated
spaceFiles
}

private def getBytesPerSpace(spaceId: UUID, user: models.User) : Long = {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this will be slow if there is a few million files in a space, can we cache this at the space level? We already save:

  collectionCount: 0,
  datasetCount: 5,
  userCount: 22,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've changed this so that it's another field in the Space object. It gets incremented/decremented as files/datasets are added or deleted.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see where that field is implemented and changed which is good - but on the main space page you're still calling this block of code to calculate dynamically right? collections/Spaces line 656 - you should be able to check the cached property of the space instead now I think?

Comment thread app/views/spaces/statistics.scala.html Outdated
<div class="panel-body">
<ul class="list-group">

@* <li class="list-group-item" title="@humanReadableNumber(spacesCount) @Messages("spaces.title")">*@
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if not used, please remove commented out code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is now removed.

@robkooper robkooper marked this pull request as draft August 24, 2021 15:18
@tcnichol tcnichol marked this pull request as ready for review August 29, 2021 19:20
Comment thread app/controllers/Spaces.scala Outdated
spaceFiles
}

private def getBytesPerSpace(spaceId: UUID, user: models.User) : Long = {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see where that field is implemented and changed which is good - but on the main space page you're still calling this block of code to calculate dynamically right? collections/Spaces line 656 - you should be able to check the cached property of the space instead now I think?

@tcnichol
Copy link
Copy Markdown
Contributor Author

@max-zilla will fix that and make another commit

@tcnichol
Copy link
Copy Markdown
Contributor Author

@max-zilla I fixed the part on app/controllers.

That should be done, I do need to test and commit the update script.

@tcnichol
Copy link
Copy Markdown
Contributor Author

Also committed the db update for adding space bytes to existing spaces.

Copy link
Copy Markdown
Member

@robkooper robkooper left a comment

Choose a reason for hiding this comment

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

I will make some changes, this breaks if the file is "imported" and not uploaded. This also breaks in case of large databases (cursor times out).

robkooper and others added 4 commits October 1, 2021 16:14
- no datasets in space would result in not writing out count
- very large databsae would have cursor timeout
- simplified logic to count bytes
* make space layout more like datasets/files

* update changelog

- undo removal
- add comment + link to issue for space stats
- comment about new layout

* Lowered margin from 20px to 15px for button links.

Co-authored-by: Luigi Marini <lmarini@illinois.edu>
@lmarini lmarini requested review from lmarini and robkooper October 4, 2021 20:14
@lmarini lmarini merged commit 1ce57e4 into develop Oct 4, 2021
@lmarini lmarini deleted the 199-show-space-bytes branch October 4, 2021 20:19
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.

show per space byte size

4 participants