[RHCLOUD-35790] Re-add remote cache event deduplication #4287
[RHCLOUD-35790] Re-add remote cache event deduplication #4287jessicarod7 wants to merge 7 commits intoRedHatInsights:masterfrom
Conversation
Not yet reimplemented with Mutiny API
Assisted-by: claude-sonnet-4-5 / Claude Code
📝 WalkthroughWalkthroughAdds optional Valkey (Redis-compatible) in-memory DB and a Valkey-based event deduplicator to the notifications engine. Introduces config flags and a template parameter to enable in-memory DB and Valkey deduplication, a new ValkeyService, conditional routing in EventDeduplicator, test lifecycle support, and tests. Changes
Sequence DiagramsequenceDiagram
participant EC as EventConsumer
participant ED as EventDeduplicator
participant CFG as EngineConfig
participant VS as ValkeyService
participant Redis as Redis/Valkey
participant DB as PostgreSQL
EC->>ED: isNew(event)
ED->>CFG: isInMemoryDbEnabled()?
alt in-memory enabled
ED->>CFG: isValkeyEventDeduplicatorEnabled()?
alt valkey dedup enabled
ED->>VS: isNewEvent(eventTypeId, dedupKey, deleteAfter, eventId)
VS->>Redis: SETNX key value
alt SETNX succeeded
Redis-->>VS: true
VS->>Redis: EXPIREAT key epochSeconds
Redis-->>VS: OK/ack
VS-->>ED: true
else already exists
Redis-->>VS: false
VS-->>ED: false
end
else valkey dedup disabled
ED->>DB: INSERT ... ON CONFLICT DO NOTHING
DB-->>ED: inserted / conflict
end
else in-memory disabled
ED->>DB: INSERT ... ON CONFLICT DO NOTHING
DB-->>ED: inserted / conflict
end
ED-->>EC: boolean (isNew)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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)
Comment |
SC Environment Impact AssessmentOverall Impact: 🟢 LOW View full reportSummary
Detailed Findings🟢 LOW ImpactFeature flag change detected
Required Actions
This assessment was automatically generated. Please review carefully and consult with the ROSA Core team for critical/high impact changes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
engine/src/test/java/com/redhat/cloud/notifications/events/EventConsumerTest.java (1)
301-329:⚠️ Potential issue | 🟠 MajorTest does not actually exercise the Valkey code path.
The test stubs
isValkeyEventDeduplicatorEnabled()but notisInMemoryDbEnabled(). SinceEventDeduplicator.isNew()requires both flags to be true to use Valkey (seeengine/src/main/java/com/redhat/cloud/notifications/events/deduplication/EventDeduplicator.javalines 59-73), andisInMemoryDbEnabled()defaults tofalse, both parameterized cases (trueandfalse) take the SQL deduplication path.To properly test the Valkey path, stub both flags:
Proposed fix to test both code paths
`@ParameterizedTest` `@ValueSource`(booleans = {true, false}) void testDuplicatePayload(final boolean valkeyDedupEnabled) { + when(config.isInMemoryDbEnabled()).thenReturn(valkeyDedupEnabled); when(config.isValkeyEventDeduplicatorEnabled()).thenReturn(valkeyDedupEnabled);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/test/java/com/redhat/cloud/notifications/events/EventConsumerTest.java` around lines 301 - 329, The test testDuplicatePayload only stubs config.isValkeyEventDeduplicatorEnabled() but not config.isInMemoryDbEnabled(), so both parameterized runs still exercise the SQL path; update the test to also stub config.isInMemoryDbEnabled() to the same valkeyDedupEnabled value (i.e., add a when(config.isInMemoryDbEnabled()).thenReturn(valkeyDedupEnabled);) so EventDeduplicator.isNew(...) will take the Valkey path when valkeyDedupEnabled is true and the SQL path when false.
🧹 Nitpick comments (2)
engine/src/main/java/com/redhat/cloud/notifications/events/ValkeyService.java (1)
27-27: Unused constantNOT_USED.This constant is defined but never referenced in the code.
Remove unused constant
- private static final String NOT_USED = "";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/main/java/com/redhat/cloud/notifications/events/ValkeyService.java` at line 27, Remove the dead constant NOT_USED from the ValkeyService class: locate the private static final String NOT_USED declaration in ValkeyService and delete it since it is never referenced; ensure no other code expects this symbol and run a build/static analysis to confirm there are no remaining unused-import or unused-symbol warnings.engine/src/test/java/com/redhat/cloud/notifications/events/ValkeyServiceTest.java (1)
40-43: Also assert that already-expired keys do not linger.This only checks the first insert returns
true. IfisNewEvent()accidentally stores a past-due key without expiring it immediately, this test still passes while later deliveries with the same dedup key would be dropped forever. Re-insertingdedupKey3once more and expectingtruewould cover that edge case.Suggested test extension
LocalDateTime expiredDeleteAfter = LocalDateTime.of(2023, 5, 11, 15, 40, 21); String dedupKey3 = "dedup-key-new-" + UUID.randomUUID(); assertTrue(valkeyService.isNewEvent(eventTypeId1, dedupKey3, expiredDeleteAfter, eventId)); + assertTrue(valkeyService.isNewEvent(eventTypeId1, dedupKey3, expiredDeleteAfter, eventId));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/test/java/com/redhat/cloud/notifications/events/ValkeyServiceTest.java` around lines 40 - 43, Extend the ValkeyServiceTest to ensure expired dedup keys are not retained: after the existing assertTrue(valkeyService.isNewEvent(eventTypeId1, dedupKey3, expiredDeleteAfter, eventId)), call valkeyService.isNewEvent(...) again with the same dedupKey3, expiredDeleteAfter and eventId and assertTrue on the second call as well; this verifies that the past-due key was not persisted and subsequent inserts with the same key still return true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.rhcicd/clowdapp-engine.yaml:
- Line 23: Add an environment variable mapping so the Java app receives the
in-memory-db.enabled flag from the same IN_MEMORY_DB_ENABLED value used for
inMemoryDb; update clowdapp-engine.yaml to inject an env var mapping that sets
the application property in-memory-db.enabled (e.g., export IN_MEMORY_DB_ENABLED
or map in-memory-db.enabled to the IN_MEMORY_DB_ENABLED value) into the
deployment's container environment so the Java application sees
in-memory-db.enabled=true when IN_MEMORY_DB_ENABLED is true.
In `@engine/pom.xml`:
- Around line 123-127: Remove the unused SmallRye Mutiny Vert.x web client
dependency: delete the dependency block with artifactId
"smallrye-mutiny-vertx-web-client" from the POM because Redis/Valkey
functionality is provided by the existing "quarkus-redis-client" (which supplies
io.vertx.mutiny.redis.client.Redis and RedisAPI used by ValkeyService); ensure
no other modules reference smallrye-mutiny-vertx-web-client before removing it
and run a build to confirm no compilation issues.
In
`@engine/src/main/java/com/redhat/cloud/notifications/events/deduplication/EventDeduplicator.java`:
- Around line 59-60: The Valkey branch in EventDeduplicator calls
valkeyService.isNewEvent(...) while still inside a DB transaction, causing
Valkey writes to become permanent even if the DB transaction later rolls back;
make this reservation rollback-safe by deferring finalization or cleaning up on
rollback: modify EventDeduplicator (the branch guarded by
engineConfig.isInMemoryDbEnabled() &&
engineConfig.isValkeyEventDeduplicatorEnabled()) to call a non-final "reserve"
method on valkeyService (e.g., valkeyService.reserveEventKey(...)) and then
register a transaction synchronization (via Spring's
TransactionSynchronizationManager.registerSynchronization or
TransactionSynchronization) that on afterCommit finalizes the reservation (e.g.,
valkeyService.confirmReservation(...)) and on afterCompletion/rollback removes
the reservation (e.g., valkeyService.cancelReservation(...)); alternatively,
perform the final write only in an onCommit callback so Valkey state mirrors DB
commit.
In
`@engine/src/main/java/com/redhat/cloud/notifications/events/ValkeyService.java`:
- Around line 50-63: The initialize() method can leave the field valkey null
when config.isInMemoryDbEnabled() is true but valkeyHost is empty, causing a
NullPointerException later in EventDeduplicator.isNew() -> isNewEvent(); fix by
either making initialize() fail-fast (throw a clear IllegalStateException with a
descriptive message when isInMemoryDbEnabled() && (valkeyHost.isEmpty() ||
valkeyHost.get().isEmpty())) or by adding a null-guard in
EventDeduplicator.isNew()/isNewEvent() that checks valkey (or
config.isInMemoryDbEnabled()) and either returns a safe default or throws a
clear exception; locate the code around ValkeyService.initialize(), the valkey
and valkeyClient fields, and EventDeduplicator.isNew()/isNewEvent() to implement
the chosen approach.
In
`@engine/src/test/java/com/redhat/cloud/notifications/events/deduplication/EventDeduplicatorTest.java`:
- Around line 70-73: The test currently only stubs
config.isValkeyEventDeduplicatorEnabled() leaving isInMemoryDbEnabled() false,
so EventDeduplicator.isNew() never takes the Valkey branch; update
EventDeduplicatorTest (methods testIsNewWithDefaultDeduplication and
testIsNewWithSubscriptionsDeduplication) to stub both
config.isValkeyEventDeduplicatorEnabled() and config.isInMemoryDbEnabled() for
each parameterized case (i.e., set isInMemoryDbEnabled() to true when valkey is
true and to false when valkey is false) so the boolean combinations exercise the
Valkey (in-memory) and SQL paths in EventDeduplicator.isNew().
In
`@engine/src/test/java/com/redhat/cloud/notifications/TestLifecycleManager.java`:
- Around line 49-55: The TestLifecycleManager.start() creates a valkeyContainer
via setupValkey but stop() currently only tears down PostgreSQL; update
TestLifecycleManager.stop() to check for the valkeyContainer field (created by
setupValkey/start) and stop/close/remove it (e.g., call stop() or close()) and
null it to avoid Docker resource leaks; also apply the same teardown in the
other stop path referenced around the second occurrence (the block covering
lines 63-69) so all code paths that start valkeyContainer properly stop it.
---
Outside diff comments:
In
`@engine/src/test/java/com/redhat/cloud/notifications/events/EventConsumerTest.java`:
- Around line 301-329: The test testDuplicatePayload only stubs
config.isValkeyEventDeduplicatorEnabled() but not config.isInMemoryDbEnabled(),
so both parameterized runs still exercise the SQL path; update the test to also
stub config.isInMemoryDbEnabled() to the same valkeyDedupEnabled value (i.e.,
add a when(config.isInMemoryDbEnabled()).thenReturn(valkeyDedupEnabled);) so
EventDeduplicator.isNew(...) will take the Valkey path when valkeyDedupEnabled
is true and the SQL path when false.
---
Nitpick comments:
In
`@engine/src/main/java/com/redhat/cloud/notifications/events/ValkeyService.java`:
- Line 27: Remove the dead constant NOT_USED from the ValkeyService class:
locate the private static final String NOT_USED declaration in ValkeyService and
delete it since it is never referenced; ensure no other code expects this symbol
and run a build/static analysis to confirm there are no remaining unused-import
or unused-symbol warnings.
In
`@engine/src/test/java/com/redhat/cloud/notifications/events/ValkeyServiceTest.java`:
- Around line 40-43: Extend the ValkeyServiceTest to ensure expired dedup keys
are not retained: after the existing
assertTrue(valkeyService.isNewEvent(eventTypeId1, dedupKey3, expiredDeleteAfter,
eventId)), call valkeyService.isNewEvent(...) again with the same dedupKey3,
expiredDeleteAfter and eventId and assertTrue on the second call as well; this
verifies that the past-due key was not persisted and subsequent inserts with the
same key still return true.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aabd86c2-825a-4228-9448-ae713c15da01
📒 Files selected for processing (11)
.rhcicd/clowdapp-engine.yamlcommon/src/test/java/com/redhat/cloud/notifications/TestConstants.javaengine/pom.xmlengine/src/main/java/com/redhat/cloud/notifications/config/EngineConfig.javaengine/src/main/java/com/redhat/cloud/notifications/events/ValkeyService.javaengine/src/main/java/com/redhat/cloud/notifications/events/deduplication/EventDeduplicator.javaengine/src/main/resources/application.propertiesengine/src/test/java/com/redhat/cloud/notifications/TestLifecycleManager.javaengine/src/test/java/com/redhat/cloud/notifications/events/EventConsumerTest.javaengine/src/test/java/com/redhat/cloud/notifications/events/ValkeyServiceTest.javaengine/src/test/java/com/redhat/cloud/notifications/events/deduplication/EventDeduplicatorTest.java
| if (engineConfig.isInMemoryDbEnabled() && engineConfig.isValkeyEventDeduplicatorEnabled()) { | ||
| return valkeyService.isNewEvent(eventTypeId, deduplicationKey.get(), deleteAfter, event.getId()); |
There was a problem hiding this comment.
Make the Valkey dedup path rollback-safe.
isNew() still runs inside a transaction, but this branch writes to Valkey outside that transaction. If the enclosing transaction rolls back later, the Valkey key remains while the database work is undone, and a retry will be treated as a duplicate. That is a behavior change from the SQL path below, where the insert is rolled back with the transaction. Please make this reservation rollback-safe (for example by cleaning it up on rollback or only finalizing it after commit).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@engine/src/main/java/com/redhat/cloud/notifications/events/deduplication/EventDeduplicator.java`
around lines 59 - 60, The Valkey branch in EventDeduplicator calls
valkeyService.isNewEvent(...) while still inside a DB transaction, causing
Valkey writes to become permanent even if the DB transaction later rolls back;
make this reservation rollback-safe by deferring finalization or cleaning up on
rollback: modify EventDeduplicator (the branch guarded by
engineConfig.isInMemoryDbEnabled() &&
engineConfig.isValkeyEventDeduplicatorEnabled()) to call a non-final "reserve"
method on valkeyService (e.g., valkeyService.reserveEventKey(...)) and then
register a transaction synchronization (via Spring's
TransactionSynchronizationManager.registerSynchronization or
TransactionSynchronization) that on afterCommit finalizes the reservation (e.g.,
valkeyService.confirmReservation(...)) and on afterCompletion/rollback removes
the reservation (e.g., valkeyService.cancelReservation(...)); alternatively,
perform the final write only in an onCommit callback so Valkey state mirrors DB
commit.
There was a problem hiding this comment.
Not applicable, Redis does not support transaction rollbacks. Unless there's a way to implement this manually and we actually do need it.
Edit: To the reviewer, please add your thoughts on the bot's suggestion, I'm leaning no.
There was a problem hiding this comment.
@jessicarod7, you're correct that Redis/Valkey has no native transaction rollback support. The concern I raised isn't about Redis rolling back — it's about the DB transaction rolling back after the Valkey key is written, which would leave a stale key and cause a retry of the same event to be silently dropped as a "duplicate."
Manual compensation is technically achievable in Quarkus without Redis transactions — you can register a jakarta.transaction.Synchronization against the current DB transaction, whose beforeCompletion is invoked prior to the two-phase commit and whose afterCompletion is called after the transaction completes. In afterCompletion, you'd check whether the status indicates a rollback and, if so, issue a Redis DEL on the deduplication key. The afterCompletion callback runs in an undefined context, but resources can still be closed and non-transactional work (like a Redis command) can be performed.
That said, this compensation approach has its own failure mode: if the DEL itself fails (Valkey unreachable after the DB rollback), you're still in the inconsistent state. This is a fundamental dual-write problem without XA support.
The practical question is: how often do DB transaction rollbacks occur after isNew() runs but before the outer transaction commits, and are retries expected in that path? If DB rollbacks in that window are rare and/or retries are unlikely to happen within the Valkey key's TTL, the current implementation may be a reasonable trade-off. If event loss on retry after a DB rollback is unacceptable for this use case, the JTA synchronization + compensating DEL pattern would address it — though imperfectly.
Happy to defer to your judgment on whether this risk level is acceptable given your system's retry behavior.
📝 Learnings were identified and not saved (knowledge base disabled). Enable
There was a problem hiding this comment.
All that code is wrapped into a large try/catch. I think we could delete the Valkey key in the catch block.
It will let us the opportunity to replay the message later, just in case it fails because of an internal issue.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
engine/src/main/java/com/redhat/cloud/notifications/events/ValkeyService.java
Show resolved
Hide resolved
...src/test/java/com/redhat/cloud/notifications/events/deduplication/EventDeduplicatorTest.java
Show resolved
Hide resolved
engine/src/test/java/com/redhat/cloud/notifications/TestLifecycleManager.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
engine/src/test/java/com/redhat/cloud/notifications/TestLifecycleManager.java (1)
64-69:⚠️ Potential issue | 🟠 MajorGuard container teardown against null to avoid teardown-time NPEs.
Line [64] and Line [67] stop containers without null checks. If startup fails mid-way (or a container was not created),
stop()can fail and mask the root cause / leak resources.Suggested fix
`@Override` public void stop() { - if (quarkusDatasourceDevServiceEnabled) { - postgreSQLContainer.stop(); + if (quarkusDatasourceDevServiceEnabled && postgreSQLContainer != null) { + postgreSQLContainer.stop(); + postgreSQLContainer = null; } - if (quarkusValkeyDevServiceEnabled) { - valkeyContainer.stop(); + if (quarkusValkeyDevServiceEnabled && valkeyContainer != null) { + valkeyContainer.stop(); + valkeyContainer = null; } MockServerLifecycleManager.stop(); InMemoryConnector.clear(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/test/java/com/redhat/cloud/notifications/TestLifecycleManager.java` around lines 64 - 69, The teardown calls unconditionally invoke postgreSQLContainer.stop() and valkeyContainer.stop(), which can throw NPEs if the containers were never created; update the teardown to check that postgreSQLContainer != null before calling postgreSQLContainer.stop() and that valkeyContainer != null before calling valkeyContainer.stop(), keeping the existing quarkusDatasourceDevServiceEnabled and quarkusValkeyDevServiceEnabled guards.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@engine/src/test/java/com/redhat/cloud/notifications/events/deduplication/EventDeduplicatorTest.java`:
- Around line 55-68: The cleanup deletes parent rows from the bundles table
before child rows in applications, which can violate the FK from Application to
Bundle; update the afterEach() cleanup in EventDeduplicatorTest to delete from
"applications" first (using the existing entityManager.createNativeQuery that
references TEST_APP_NAME and SUBSCRIPTIONS_APP_NAME), then delete from "bundles"
(using TEST_BUNDLE_NAME and SUBSCRIPTION_SERVICES_BUNDLE_NAME), preserving the
`@Transactional` scope and reusing the same parameter names so FK constraints
won't cause failures.
---
Duplicate comments:
In
`@engine/src/test/java/com/redhat/cloud/notifications/TestLifecycleManager.java`:
- Around line 64-69: The teardown calls unconditionally invoke
postgreSQLContainer.stop() and valkeyContainer.stop(), which can throw NPEs if
the containers were never created; update the teardown to check that
postgreSQLContainer != null before calling postgreSQLContainer.stop() and that
valkeyContainer != null before calling valkeyContainer.stop(), keeping the
existing quarkusDatasourceDevServiceEnabled and quarkusValkeyDevServiceEnabled
guards.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b0b509b9-4624-4a5c-9500-b2d3a6ae63b1
📒 Files selected for processing (4)
engine/pom.xmlengine/src/main/java/com/redhat/cloud/notifications/events/ValkeyService.javaengine/src/test/java/com/redhat/cloud/notifications/TestLifecycleManager.javaengine/src/test/java/com/redhat/cloud/notifications/events/deduplication/EventDeduplicatorTest.java
✅ Files skipped from review due to trivial changes (1)
- engine/pom.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- engine/src/main/java/com/redhat/cloud/notifications/events/ValkeyService.java
...src/test/java/com/redhat/cloud/notifications/events/deduplication/EventDeduplicatorTest.java
Show resolved
Hide resolved
jessicarod7
force-pushed
the
RHCLOUD-35790
branch
from
51f5242 to
d98b13d
Compare
Assisted-by: @coderabbitai
jessicarod7
force-pushed
the
RHCLOUD-35790
branch
from
d98b13d to
501dd7a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
engine/src/test/java/com/redhat/cloud/notifications/events/deduplication/EventDeduplicatorTest.java (1)
70-75: Consider covering the full flag matrix (2x2) in both parameterized tests.Right now only
(true,true)and(false,false)are covered. Adding(true,false)and(false,true)would better guard the fallback behavior when only one flag is enabled (Line [73]-Line [74], Line [106]-Line [107]).Proposed test parameterization update
-@ValueSource(booleans = {true, false}) -void testIsNewWithDefaultDeduplication(final boolean valkeyDedupEnabled) { - when(config.isValkeyEventDeduplicatorEnabled()).thenReturn(valkeyDedupEnabled); - when(config.isInMemoryDbEnabled()).thenReturn(valkeyDedupEnabled); +@CsvSource({ + "true, true", + "true, false", + "false, true", + "false, false" +}) +void testIsNewWithDefaultDeduplication(final boolean inMemoryDbEnabled, final boolean valkeyDedupEnabled) { + when(config.isInMemoryDbEnabled()).thenReturn(inMemoryDbEnabled); + when(config.isValkeyEventDeduplicatorEnabled()).thenReturn(valkeyDedupEnabled);-@ValueSource(booleans = {true, false}) -void testIsNewWithSubscriptionsDeduplication(final boolean valkeyDedupEnabled) { - when(config.isValkeyEventDeduplicatorEnabled()).thenReturn(valkeyDedupEnabled); - when(config.isInMemoryDbEnabled()).thenReturn(valkeyDedupEnabled); +@CsvSource({ + "true, true", + "true, false", + "false, true", + "false, false" +}) +void testIsNewWithSubscriptionsDeduplication(final boolean inMemoryDbEnabled, final boolean valkeyDedupEnabled) { + when(config.isInMemoryDbEnabled()).thenReturn(inMemoryDbEnabled); + when(config.isValkeyEventDeduplicatorEnabled()).thenReturn(valkeyDedupEnabled);Also applies to: 103-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/test/java/com/redhat/cloud/notifications/events/deduplication/EventDeduplicatorTest.java` around lines 70 - 75, The parameterized tests only cover (true,true) and (false,false); update both parameterized tests (e.g., testIsNewWithDefaultDeduplication) to cover the full 2x2 flag matrix by replacing `@ValueSource`(booleans = {true,false}) with `@CsvSource`({"true,true","true,false","false,true","false,false"}) and change the test method signature to accept two boolean parameters (valkeyDedupEnabled, inMemoryDbEnabled), then call when(config.isValkeyEventDeduplicatorEnabled()).thenReturn(valkeyDedupEnabled) and when(config.isInMemoryDbEnabled()).thenReturn(inMemoryDbEnabled); apply the same change to the other parameterized test around lines 103-107 so both tests exercise all four flag combinations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@engine/src/test/java/com/redhat/cloud/notifications/events/deduplication/EventDeduplicatorTest.java`:
- Around line 70-75: The parameterized tests only cover (true,true) and
(false,false); update both parameterized tests (e.g.,
testIsNewWithDefaultDeduplication) to cover the full 2x2 flag matrix by
replacing `@ValueSource`(booleans = {true,false}) with
`@CsvSource`({"true,true","true,false","false,true","false,false"}) and change the
test method signature to accept two boolean parameters (valkeyDedupEnabled,
inMemoryDbEnabled), then call
when(config.isValkeyEventDeduplicatorEnabled()).thenReturn(valkeyDedupEnabled)
and when(config.isInMemoryDbEnabled()).thenReturn(inMemoryDbEnabled); apply the
same change to the other parameterized test around lines 103-107 so both tests
exercise all four flag combinations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 79c98110-41d1-4cfb-8f50-022b9133cd6d
📒 Files selected for processing (4)
engine/pom.xmlengine/src/main/java/com/redhat/cloud/notifications/events/ValkeyService.javaengine/src/test/java/com/redhat/cloud/notifications/TestLifecycleManager.javaengine/src/test/java/com/redhat/cloud/notifications/events/deduplication/EventDeduplicatorTest.java
✅ Files skipped from review due to trivial changes (1)
- engine/pom.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- engine/src/main/java/com/redhat/cloud/notifications/events/ValkeyService.java
- engine/src/test/java/com/redhat/cloud/notifications/TestLifecycleManager.java
| if (engineConfig.isInMemoryDbEnabled() && engineConfig.isValkeyEventDeduplicatorEnabled()) { | ||
| return valkeyService.isNewEvent(eventTypeId, deduplicationKey.get(), deleteAfter, event.getId()); |
There was a problem hiding this comment.
All that code is wrapped into a large try/catch. I think we could delete the Valkey key in the catch block.
It will let us the opportunity to replay the message later, just in case it fails because of an internal issue.
| UUID eventTypeId = event.getEventType().getId(); | ||
| LocalDateTime deleteAfter = eventDeduplicationConfig.getDeleteAfter(event); | ||
|
|
||
| if (engineConfig.isInMemoryDbEnabled() && engineConfig.isValkeyEventDeduplicatorEnabled()) { |
There was a problem hiding this comment.
We need to "feed" Valkey for 15 days before using it for dedup check.
could you also compare both result valkey and RDS, then log a warn if results are different ?
(RDS should have the last word for few weeks, then we could drop it if result are always identical for 2 weeks)
| .setParameter("testName", TEST_APP_NAME) | ||
| .setParameter("subName", SUBSCRIPTIONS_APP_NAME) | ||
| .executeUpdate(); | ||
| entityManager |
There was a problem hiding this comment.
Deleting bundles will cascade delete applications.
2 participants
Jira work item
https://redhat.atlassian.net/browse/RHCLOUD-35790
Description
This re-adds support for event deduplication via remote caches, implemented with Valkey. It works identically to the PostgreSQL database table, and depends on two parameters:
IN_MEMORY_DB_ENABLED: instructs Clowder to create a Valkey instance to store the data innotifications-engine.valkey-event-deduplicator.enabled: Unleash toggle to switch to using Valkey for event dedupDeduplication is done with keys in the format
And additionally stores
deleteAfteras a value so it is internally similar to Postgres. If the event has not been seen before, the key is set to expire at this time.Improvements since last time:
Compatibilty
No impact unless both of the above feature flags are enabled.
Testing
Summary by CodeRabbit
New Features
Tests