New Population script + Clustering #166
Merged
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 8 minutes and 50 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughNew Spring component LocationProcessorWorker runs on ApplicationReadyEvent, wraps work in a transaction, deletes all rows from store_inventory_map, then repopulates it via a single CTE-based INSERT that clusters, aggregates, and computes geometric medians from raw_user_pings; logs results and rethrows exceptions. Changes
Sequence Diagram(s)sequenceDiagram
participant App as ApplicationReadyEvent
participant Worker as LocationProcessorWorker
participant JDBC as JdbcTemplate
participant DB as Postgres/PostGIS
App->>Worker: trigger processAndCalculateCenters()
Worker->>Worker: log start
Worker->>JDBC: execute DELETE FROM store_inventory_map
JDBC->>DB: DELETE query
DB-->>JDBC: delete result
Worker->>JDBC: execute INSERT ... WITH CTEs (filter pings, ST_ClusterDBSCAN, ST_GeometricMedian, aggregates)
JDBC->>DB: complex INSERT/SELECT
DB-->>JDBC: inserted/updated count
JDBC-->>Worker: rows updated
Worker->>Worker: log updated count
alt error occurs
Worker->>Worker: log error
Worker-->>App: rethrow exception (transaction rollback)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/main/java/com/p2ps/service/LocationProcessorWorker.java (1)
23-23: ReplaceSystem.out/System.errwith a proper logger.Using
System.out.printlnandSystem.err.printlnbypasses the logging framework, making it harder to configure log levels, aggregate logs, and monitor the application in production.Proposed fix
Add import and logger field:
import org.slf4j.Logger; import org.slf4j.LoggerFactory;`@Component` public class LocationProcessorWorker { + private static final Logger log = LoggerFactory.getLogger(LocationProcessorWorker.class); private final JdbcTemplate jdbcTemplate;Then replace the print statements:
- System.out.println("⏳ [Worker] Începem recalcularea centrelor absolute..."); + log.info("Starting absolute center recalculation...");- System.out.println("✅ [Worker] Procesare finalizată cu succes! Au fost actualizate " + rowsAffected + " locații la raft."); + log.info("Processing completed successfully. Updated {} shelf locations.", rowsAffected);- System.err.println("❌ [Worker] Eroare la procesarea locațiilor: " + e.getMessage()); + log.error("Error processing locations", e);Also applies to: 61-61, 64-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/p2ps/service/LocationProcessorWorker.java` at line 23, Replace the System.out/err prints in LocationProcessorWorker with an SLF4J logger: add imports for org.slf4j.Logger and org.slf4j.LoggerFactory and declare a private static final Logger (e.g., LOGGER) on the class, then replace calls like System.out.println("⏳ [Worker] Începem recalcularea centrelor absolute...") and other prints (the ones around the 61 and 64 occurrences) with LOGGER.info(...) or LOGGER.error(...) as appropriate so logging goes through the logging framework.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/p2ps/service/LocationProcessorWorker.java`:
- Line 27: The current jdbcTemplate.execute("TRUNCATE store_inventory_map") call
in LocationProcessorWorker causes read disruption; replace this with a safer
swap or transactional approach: create/use a staging table (e.g.,
store_inventory_map_staging), insert/update all rows there via the existing
insert logic, then perform an atomic rename/swap of tables or use a
transactional DELETE+INSERT on store_inventory_map if volume allows; update the
code that calls jdbcTemplate.execute to instead write to the staging table and
then perform the rename/DELETE+INSERT step so readers never see an empty table
during refresh.
- Around line 44-56: The query groups by c.cluster_id which can produce multiple
rows per (store_id, item_id) and violate the UNIQUE(store_id, item_id)
constraint; to fix, remove c.cluster_id from the GROUP BY in
LocationProcessorWorker.java's SELECT block and aggregate all cluster points per
(c.store_id, c.item_id) (keep ST_GeometricMedian(ST_Collect(c.location_point)),
COUNT(c.item_id), AVG(c.accuracy_m) as aggregates) so the query returns a single
row per (store_id, item_id); alternatively, if multiple clusters must be
preserved, update the schema to remove or adjust the UNIQUE(store_id, item_id)
constraint instead.
- Line 1: The package declaration at the top of the file is incorrect for the
class LocationProcessorWorker; update the package declaration (replace the
current "org.example") to the correct package name used by the project (e.g.,
"com.p2ps.service") so Spring component scanning can find
LocationProcessorWorker and any annotations on that class will be recognized.
- Line 31: The INSERT into store_inventory_map in LocationProcessorWorker.java
references a non-existent column formatted_cluster_id and the SELECT that feeds
it groups by c.cluster_id which can emit multiple rows per (store_id,item_id)
violating the UNIQUE(store_id,item_id) constraint; fix by removing
formatted_cluster_id from the INSERT column list and from the SELECT projection,
and change the SELECT aggregation to collapse rows per store_id,item_id (e.g.,
GROUP BY s.store_id, i.item_id and use aggregate functions like
MAX(c.confidence_score) and SUM(c.ping_count) or choose a single cluster via MAX
or a DISTINCT ON/select-first pattern) so only one row per (store_id,item_id) is
produced and the INSERT will succeed without schema changes.
---
Nitpick comments:
In `@src/main/java/com/p2ps/service/LocationProcessorWorker.java`:
- Line 23: Replace the System.out/err prints in LocationProcessorWorker with an
SLF4J logger: add imports for org.slf4j.Logger and org.slf4j.LoggerFactory and
declare a private static final Logger (e.g., LOGGER) on the class, then replace
calls like System.out.println("⏳ [Worker] Începem recalcularea centrelor
absolute...") and other prints (the ones around the 61 and 64 occurrences) with
LOGGER.info(...) or LOGGER.error(...) as appropriate so logging goes through the
logging framework.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d93a805d-3376-41ff-b27b-8bfda2659815
📒 Files selected for processing (2)
init-scripts/POPULARE NOU.txtsrc/main/java/com/p2ps/service/LocationProcessorWorker.java
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 4 unresolved review comments. A stacked PR containing fixes has been created.
Time taken: |
Updated the location processing logic to delete old entries before inserting new ones and improved the SQL query to select unique locations based on the highest ping count.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/com/p2ps/service/LocationProcessorWorker.java (1)
20-25: Replace System.out/err with structured logging using SLF4J.Lines 20, 25, 68 use
System.out.println()and line 71 usesSystem.err.println()with onlye.getMessage(), losing stack trace context. The codebase establishes SLF4J as the standard logging approach across all services.Proposed logging refactor
+import org.slf4j.Logger; +import org.slf4j.LoggerFactory; ... public class LocationProcessorWorker { + private static final Logger log = LoggerFactory.getLogger(LocationProcessorWorker.class); ... - System.out.println("⏳ [Worker] Începem recalcularea centrelor absolute..."); + log.info("⏳ [Worker] Începem recalcularea centrelor absolute..."); ... - System.out.println("ℹ️ [Worker] Am șters " + deletedRows + " locații vechi."); + log.info("ℹ️ [Worker] Am șters {} locații vechi.", deletedRows); ... - System.out.println("✅ [Worker] Tranzacție finalizată! Au fost actualizate atomic " + insertedRows + " locații unice la raft."); + log.info("✅ [Worker] Tranzacție finalizată! Au fost actualizate atomic {} locații unice la raft.", insertedRows); ... - System.err.println("❌ [Worker] Eroare la procesarea locațiilor: " + e.getMessage()); + log.error("❌ [Worker] Eroare la procesarea locațiilor.", e); throw e;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/p2ps/service/LocationProcessorWorker.java` around lines 20 - 25, Replace all System.out.println/System.err.println calls in LocationProcessorWorker with a SLF4J logger: add a private static final Logger logger = LoggerFactory.getLogger(LocationProcessorWorker.class) at class scope, change the info prints (e.g., "⏳ [Worker] Începem...", "ℹ️ [Worker] Am șters ...") to logger.info(...) and change error prints (where exceptions are caught and e.getMessage() is logged) to logger.error("Descriptive message", e) so the full stack trace is preserved; update every occurrence in the class (lines with System.out/System.err) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/p2ps/service/LocationProcessorWorker.java`:
- Around line 56-63: The DISTINCT ON query in LocationProcessorWorker.java
selecting from ClusterStats uses ORDER BY store_id, item_id, ping_count DESC
which can leave ties unresolved; update the ORDER BY used with DISTINCT ON
(store_id, item_id) to include deterministic tie-breakers (for example: add
confidence_score DESC, estimated_loc_point ASC, and finally the table's unique
identifier or timestamp column such as id or created_at) so rows with equal
ping_count are ordered deterministically and the same cluster is chosen every
run.
- Around line 17-19: The scheduled worker method processAndCalculateCenters
currently uses `@Scheduled`(fixedRate = 300000) which can overlap; change to
`@Scheduled`(fixedDelay = 300000) on the processAndCalculateCenters method so each
run starts only after the previous finishes, and if the app runs in multiple
instances add a distributed lock (e.g., integrate ShedLock on
processAndCalculateCenters with `@SchedulerLock`) to ensure a single node performs
the DELETE/INSERT operations on store_inventory_map at a time.
---
Nitpick comments:
In `@src/main/java/com/p2ps/service/LocationProcessorWorker.java`:
- Around line 20-25: Replace all System.out.println/System.err.println calls in
LocationProcessorWorker with a SLF4J logger: add a private static final Logger
logger = LoggerFactory.getLogger(LocationProcessorWorker.class) at class scope,
change the info prints (e.g., "⏳ [Worker] Începem...", "ℹ️ [Worker] Am șters
...") to logger.info(...) and change error prints (where exceptions are caught
and e.getMessage() is logged) to logger.error("Descriptive message", e) so the
full stack trace is preserved; update every occurrence in the class (lines with
System.out/System.err) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5a53b290-45a9-4518-8ff4-87cdc52ef218
📒 Files selected for processing (1)
src/main/java/com/p2ps/service/LocationProcessorWorker.java
Changed the scheduling from fixedRate to fixedDelay to ensure the 5-minute timer starts only after the method completes successfully.
Added @EnableSchedualing because of the worker class that builds the clustering algo
Removed the @EnableScheduling annotation from the application.
Changed the scheduling of the processAndCalculateCenters method to use fixedDelay instead of fixedRate, ensuring the method runs after the previous execution completes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/com/p2ps/service/LocationProcessorWorker.java (1)
54-61:⚠️ Potential issue | 🟡 Minor
DISTINCT ONtie-breaking is still nondeterministic for equalping_count.Line 61 orders only by
ping_count DESC. If two clusters tie onping_count, the selected row can vary run-to-run.Proposed fix
- ORDER BY store_id, item_id, ping_count DESC + ORDER BY store_id, item_id, ping_count DESC, confidence_score DESC, cluster_id ASC🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/p2ps/service/LocationProcessorWorker.java` around lines 54 - 61, The SELECT using DISTINCT ON in LocationProcessorWorker returns nondeterministic results when ping_count ties because the ORDER BY only includes ping_count DESC; update the ORDER BY used with DISTINCT ON (the query selecting from ClusterStats) to add a deterministic tie-breaker (for example add confidence_score DESC and then a stable unique field such as cluster_id or estimated_loc_point) so rows with equal ping_count are consistently ordered and the same row is chosen each run.
🧹 Nitpick comments (2)
src/main/java/com/p2ps/service/LocationProcessorWorker.java (1)
22-23: Prefer structured logger overSystem.out/System.errin Spring components.At Lines 22, 26, 66, and 69, use SLF4J logging (
log.info/error) instead of console prints for production observability and stack traces.Also applies to: 26-27, 66-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/p2ps/service/LocationProcessorWorker.java` around lines 22 - 23, Replace the console prints in the LocationProcessorWorker class with an SLF4J logger: add a private static final Logger (LoggerFactory.getLogger(LocationProcessorWorker.class)) to the class and change all System.out.println calls (lines showing "Începem recalcularea centrelor absolute..." and other info messages) to log.info(...), and change any System.err/stack-trace printing to log.error(..., exception) so exceptions are logged with their stack traces; ensure the log messages match the original text and include the exception object where one is available.src/test/java/com/p2ps/service/LocationProcessorWorkerTest.java (1)
35-36: Strengthen interaction assertions with ordered SQL intent checks.Current
times(2)onanyString()is broad; it won’t catch regressions like DELETE+DELETE. Consider asserting call order and prefixes (DELETE first, INSERT second).Example improvement
+ InOrder inOrder = inOrder(jdbcTemplate); + inOrder.verify(jdbcTemplate).update(startsWith("DELETE FROM store_inventory_map")); + inOrder.verify(jdbcTemplate).update(startsWith("INSERT INTO store_inventory_map")); + inOrder.verifyNoMoreInteractions(); - verify(jdbcTemplate, times(2)).update(anyString());Also applies to: 52-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/p2ps/service/LocationProcessorWorkerTest.java` around lines 35 - 36, Replace the broad verify(jdbcTemplate, times(2)).update(anyString()) assertions in LocationProcessorWorkerTest with ordered, intentful checks: create an InOrder for jdbcTemplate and assert the first call to update(...) has a SQL string that starts with "DELETE" (or matches the delete prefix) and the second call starts with "INSERT" (or matches the insert prefix); use Mockito.inOrder(jdbcTemplate).verify(jdbcTemplate).update(argThat(s -> s.startsWith("DELETE"))) and then .verify(...).update(argThat(s -> s.startsWith("INSERT"))) to ensure call order and intent (apply the same change for the other occurrence at lines 52-53).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/p2ps/service/LocationProcessorWorker.java`:
- Around line 19-21: processAndCalculateCenters() is currently only wired to run
once via `@EventListener`(ApplicationReadyEvent.class); add a periodic trigger by
annotating the same method with `@Scheduled`(fixedDelay = <milliseconds>) (keep
the existing `@EventListener` if you want startup run too) and ensure the
scheduling infrastructure is enabled by adding `@EnableScheduling` on an existing
`@Configuration` class (or create one) so Spring will honor `@Scheduled`; update
imports to include org.springframework.scheduling.annotation.Scheduled and
org.springframework.scheduling.annotation.EnableScheduling as needed.
---
Duplicate comments:
In `@src/main/java/com/p2ps/service/LocationProcessorWorker.java`:
- Around line 54-61: The SELECT using DISTINCT ON in LocationProcessorWorker
returns nondeterministic results when ping_count ties because the ORDER BY only
includes ping_count DESC; update the ORDER BY used with DISTINCT ON (the query
selecting from ClusterStats) to add a deterministic tie-breaker (for example add
confidence_score DESC and then a stable unique field such as cluster_id or
estimated_loc_point) so rows with equal ping_count are consistently ordered and
the same row is chosen each run.
---
Nitpick comments:
In `@src/main/java/com/p2ps/service/LocationProcessorWorker.java`:
- Around line 22-23: Replace the console prints in the LocationProcessorWorker
class with an SLF4J logger: add a private static final Logger
(LoggerFactory.getLogger(LocationProcessorWorker.class)) to the class and change
all System.out.println calls (lines showing "Începem recalcularea centrelor
absolute..." and other info messages) to log.info(...), and change any
System.err/stack-trace printing to log.error(..., exception) so exceptions are
logged with their stack traces; ensure the log messages match the original text
and include the exception object where one is available.
In `@src/test/java/com/p2ps/service/LocationProcessorWorkerTest.java`:
- Around line 35-36: Replace the broad verify(jdbcTemplate,
times(2)).update(anyString()) assertions in LocationProcessorWorkerTest with
ordered, intentful checks: create an InOrder for jdbcTemplate and assert the
first call to update(...) has a SQL string that starts with "DELETE" (or matches
the delete prefix) and the second call starts with "INSERT" (or matches the
insert prefix); use
Mockito.inOrder(jdbcTemplate).verify(jdbcTemplate).update(argThat(s ->
s.startsWith("DELETE"))) and then .verify(...).update(argThat(s ->
s.startsWith("INSERT"))) to ensure call order and intent (apply the same change
for the other occurrence at lines 52-53).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b616103-8e1e-4229-a4ad-b10e36ad77eb
📒 Files selected for processing (6)
init-scripts/POPULARE NOU.sqlinit-scripts/SCRIPT_POPULARE.sqlsrc/main/java/com/p2ps/P2PShoppingApplication.javasrc/main/java/com/p2ps/service/LocationProcessorWorker.javasrc/test/java/com/p2ps/service/LocationProcessorWorkerTest.javasrc/test/resources/schema.sql
✅ Files skipped from review due to trivial changes (2)
- src/main/java/com/p2ps/P2PShoppingApplication.java
- src/test/resources/schema.sql
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/test/java/com/p2ps/auth/AuthControllerTest.java`:
- Around line 47-53: The test containers in AuthControllerTest use non-pinned
images (mongoDBContainer and redisContainer) with "mongo:latest" and
"redis:latest"; change these to specific, immutable tags (for example
"mongo:6.0.14" and "redis:7.2.1" or whatever project-approved versions) to
ensure reproducible tests, updating the GenericContainer and MongoDBContainer
instantiations to use the chosen tags and run the test suite to confirm
compatibility.
- Around line 55-64: The test's DynamicPropertySource method dynamicProperties
registers wrong property names so the containers aren't wired into the app;
update the registry.add calls in dynamicProperties to use the real keys used by
the application: replace "spring.data.mongodb.uri" with "spring.mongodb.uri",
and replace "spring.redis.host" and "spring.redis.port" with
"spring.data.redis.host" and "spring.data.redis.port" respectively so the test
uses the container endpoints at runtime.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 202b3504-4373-4204-bdb1-497bb4e1b2b5
📒 Files selected for processing (3)
build.gradle.ktsinit-scripts/03-init-spatial.sqlsrc/test/java/com/p2ps/auth/AuthControllerTest.java
✅ Files skipped from review due to trivial changes (1)
- build.gradle.kts
bmbianca
force-pushed
the
87-spatial-clustering-engine
branch
from
0e4e7aa to
f2529f6
Compare
|
This was referenced Apr 16, 2026
This was referenced Apr 21, 2026
Merged
This was referenced May 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changed the population script because of too much noise and the clustering script / algorithm did not work properly
Added LocationProccessWorker class to atomate the clustering algorithm every X minutes
If you want to check it out just run the population script and then after you open the aplication the class should start by itself
Closes P2P-Shopping/server/#87
Summary by CodeRabbit