-
Notifications
You must be signed in to change notification settings - Fork 757
Track storage #1960
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
Track storage #1960
Conversation
To keep the write load under control when enqueuing a ton of these
| @@ -0,0 +1,19 @@ | |||
| module ActionTextContentBytesUsed | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This extends ActionText objects to query for the used storage.
262f62d to
70fc030
Compare
70fc030 to
6b4c02e
Compare
flavorjones
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏
| @@ -0,0 +1,19 @@ | |||
| module ActionTextContentStorageTracking | |||
| def bytes_used | |||
| attachables.sum { |attachable| attachable.try(:byte_size) || 0 } | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use attachable_filesize here which tries both byte_size and filesize:
| attachables.sum { |attachable| attachable.try(:byte_size) || 0 } | |
| attachables.sum { |attachable| attachable.attachable_filesize || 0 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nice TIL
|
Should we display "storage used" somewhere on the Account Settings page? |
I was trying to get the internals here, but yeah that will be the next step once this gets merged (cc @jzimdars) |
2cbdd4f to
8f48b89
Compare
adb2a27 to
9ac37fb
Compare
So that the system is more robust and it is faster to recalculate storage taken by account
9ac37fb to
1e2e9e1
Compare
| def count_bytes_used | ||
| total_bytes = 0 | ||
|
|
||
| cards.with_rich_text_description_and_embeds.find_each do |card| | ||
| total_bytes += card.bytes_used | ||
| total_bytes += card.comments.with_rich_text_body_and_embeds.sum(&:bytes_used) | ||
| end | ||
|
|
||
| total_bytes | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I think we'd just iterate over the cards and ask them to recalculate themselves, taking care of their own dependents.
Each class that includes ::StorageTracking is responsible for providing recalculate_bytes_used and having it return the new value. They can implement their own caching policy as they see fit. Cards and comments, for example, don't cache the value, but having them do so would be easy to add.
# Account::StorageTracking
def recalculate_bytes_used
boards.find_each(&:recalculate_bytes_used).sum.tap do |value|
update_column :bytes_used, value
end
end
# Board::StorageTracking
def recalculate_bytes_used
cards.find_each(&:recalculate_bytes_used).sum.tap do |value|
update_column :bytes_used, value
end
end
# Card::StorageTracking (doesn't cache the result)
def recalculate_bytes_used
bytes_used = rich_text_associations.sum { # ... }
bytes_used + comments.find_each(&:recalculate_bytes_used).sum
endIf we need to in the future, we could implement a more efficient calculate_bytes_used. It would use cached values where possible without performing a full recalculation.
This tracks storage at the account and board level.
Comments, cards, boards will notify up storage changes, offering an opportunity to store the change to the ancestors in the containment hierarchy.
We only count the storage taken by Action Text attachments for now.
Thanks for @packagethief for feedback on the approach, based on our experience with Basecamp 🙏