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

Improve Edge Caching Mechanism in InitializeEdgesWorker #2827

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

Sn0w3y
Copy link
Contributor

@Sn0w3y Sn0w3y commented Oct 1, 2024

This pull request enhances the InitializeEdgesWorker class by implementing a regular caching mechanism for edges from PostgreSQL. The main improvements are:

Regular edge caching: Edges are now cached immediately upon startup and then every 20 minutes.
Simplified scheduling: Uses a ScheduledExecutorService to manage both immediate and periodic tasks.
Executor management: Ensures that executors are properly shut down when stopping the worker.
Improved error handling and logging: Enhanced logging for better error tracing and debugging.

Benefits

  • Regular Edge Caching: Keeps the edge cache updated by regularly fetching data from PostgreSQL.
  • Simplified Scheduling: Reduces complexity by using a single scheduler for task execution.
  • Resource Management: Ensures executors are properly shut down to prevent resource leaks.
  • Improved Logging: Enhanced error messages facilitate easier debugging.

Copy link

codecov bot commented Oct 1, 2024

Codecov Report

Attention: Patch coverage is 0% with 27 lines in your changes missing coverage. Please review.

Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #2827      +/-   ##
=============================================
+ Coverage      56.01%   56.10%   +0.09%     
- Complexity      8197     8544     +347     
=============================================
  Files           2103     2140      +37     
  Lines          89017    90556    +1539     
  Branches        6560     6719     +159     
=============================================
+ Hits           49854    50795     +941     
- Misses         37445    38004     +559     
- Partials        1718     1757      +39     

@sfeilmeier
Copy link
Contributor

sfeilmeier commented Oct 13, 2024

@michaelgrill: Can you please take over?

Should we switch this caching to a library that handles updating internally? Guava provides a nice Cache feature: https://github.com/google/guava/wiki/cachesexplained
Reconsiderung: no, this does not really make sense here.

@Sn0w3y: I suggest we make the refresh time configurable (time in minutes). What do you think?

@Sn0w3y
Copy link
Contributor Author

Sn0w3y commented Oct 13, 2024

@michaelgrill: Can you please take over?

Should we switch this caching to a library that handles updating internally? Guava provides a nice Cache feature: https://github.com/google/guava/wiki/cachesexplained Reconsiderung: no, this does not really make sense here.

@Sn0w3y: I suggest we make the refresh time configurable (time in minutes). What do you think?

I will add a Configurable Time to the Worker aswell as "Enable and Disable" Option :)

@sfeilmeier
Copy link
Contributor

I will add a Configurable Time to the Worker aswell as "Enable and Disable" Option :)

One value is sufficient. Negative or zero means "disable".

@Sn0w3y
Copy link
Contributor Author

Sn0w3y commented Oct 13, 2024

I will add a Configurable Time to the Worker aswell as "Enable and Disable" Option :)

One value is sufficient. Negative or zero means "disable".

Done :)

@sfeilmeier
Copy link
Contributor

I had a quick look and renamed the config property to cacheRefreshInterval. But InitializeEdgesWorker also has to be renamed, because the name does not make any sense anymore. Also Checkstyle currently fails...

@katsuya
Copy link
Contributor

katsuya commented Dec 2, 2024

@Sn0w3y Thank you so much! We’ve started testing it internally!

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.

4 participants