-
-
Notifications
You must be signed in to change notification settings - Fork 2
GH-104 Refactor baltop using database indexes. #104
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
Conversation
# Conflicts: # eternaleconomy-core/build.gradle.kts
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.
Code Review
This pull request refactors the leaderboard data flow by extracting the logic from AccountManager into a new LeaderboardServiceImpl. This is a good structural improvement that separates concerns. The new service introduces caching for the top players, which is a great step towards better performance.
However, I've identified several critical performance issues in the new implementation, primarily related to database interactions. The code currently loads large amounts of data (sometimes all accounts) into memory for sorting and filtering, which should be handled by the database using appropriate SQL queries. I've also noted a performance regression in the account prefix search functionality.
My review includes detailed suggestions on how to resolve these performance bottlenecks by leveraging database capabilities more effectively. Additionally, I've pointed out a minor dependency version inconsistency in the build configuration and a suggestion to use proper logging.
...onomy-core/src/main/java/com/eternalcode/economy/account/database/AccountRepositoryImpl.java
Show resolved
Hide resolved
...aleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardServiceImpl.java
Show resolved
Hide resolved
eternaleconomy-core/src/main/java/com/eternalcode/economy/account/AccountManager.java
Outdated
Show resolved
Hide resolved
...aleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardServiceImpl.java
Outdated
Show resolved
Hide resolved
eternaleconomy-core/src/main/java/com/eternalcode/economy/account/AccountManager.java
Show resolved
Hide resolved
…tion for leaderboard and account management. Update dependencies and `AccountManager` to improve consistency.
…ormatting and configuration consistency
|
/gemini review |
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.
Code Review
This pull request refactors the leaderboard system to improve performance and scalability. The key changes include extracting leaderboard logic from AccountManager into a new LeaderboardServiceImpl, implementing a caching layer with Caffeine, offloading leaderboard queries to the database, and making data retrieval asynchronous. The refactoring is well-executed. However, I've identified a critical regression in the account creation logic and a bug in the new leaderboard implementation. My review includes suggestions to address these points.
eternaleconomy-core/src/main/java/com/eternalcode/economy/account/AccountManager.java
Show resolved
Hide resolved
...aleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardServiceImpl.java
Show resolved
Hide resolved
…rdServiceImpl` to use accurate total entry calculations.
No description provided.