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

make the adding of all messages to the cache optional #984

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

Conversation

twonirwana
Copy link

This allows it to disable the adding of all messages to the cache and thereby reducing the resources.

@felldo
Copy link
Member

felldo commented Apr 13, 2022

Isn't this the effectively the same as DiscordApi#setMessageCacheSize(0,0) ? Even if they are cached for 1 minute as the Javadoc states, I think that isn't that bad?

@twonirwana
Copy link
Author

My bot is not that popular (on around 300 servers) and even with setMessageCacheSize(0,0) but i have up to 1000 messages in the cache and get Heap memory was too low to hold all configured messages in the cache. messages (even with 5gig of heap). I did not look at the threads but i think all the cleanup schedules will also be quite active.

@felldo
Copy link
Member

felldo commented Apr 13, 2022

There seems to be an issue with this message see #647 so this message is not 100% reliable as far as I have witnessed. Imo the best would be that the issue gets fixed instead of adding a workaround as this seems
to be when looking at the linked issue.

@twonirwana
Copy link
Author

Heap memory was too low to hold all configured messages in the cache. was only the reason i looked into the caching. I think adding hundreds of messages to a cache only to remove all of them 60s later is not a optimal usage of resources. If you are interested i could also create a pull request where i replace your self written caching with a Guava Cache, that should be more reliable.

@Bastian Bastian changed the base branch from development to master April 25, 2022 18:30
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.

2 participants