gnodet-bot commented on code in PR #29505:
URL: https://github.com/apache/camel/pull/29505#discussion_r4560899729
##########
components/camel-redis/src/test/java/org/apache/camel/component/redis/processor/aggregate/integration/RedisAggregationRepositoryOperationsIT.java:
##########
@@ -151,6 +156,131 @@ public void testOptimisticRemoveWithRecovery() {
}
}
+ @Test
+ public void testOptimisticRemoveWhenRecoveryWriteFails() {
+ String mapName = "optimisticRemoveRecoveryFailure";
+ RedisAggregationRepository repo = createRepo(mapName, true);
+
+ try {
+ // 1. Add an exchange to the active map
+ Exchange exchange = createExchange("recoverable");
+ repo.add(context, "key1", null, exchange);
+
+ // 2. Create a Redis String at the recovery map's key
+ String recoveryMapName = mapName + "-completed";
+ repo.getRedisson()
+ .getBucket(recoveryMapName)
+ .set("wrong-type");
+
+ // 3. Attempt removal; recovery write should fail
+ assertThrows(RedisException.class,
+ () -> repo.remove(context, "key1", exchange));
+
+ // 4. Verify the exchange still exists in the active map
+ Exchange result = repo.get(context, "key1");
+ assertNotNull(result);
+ assertEquals("recoverable", result.getIn().getBody(String.class));
+
+ } finally {
+ repo.stop();
+ }
+ }
+
+ @Test
+ public void testOptimisticRemoveDoesNotPopulateRecoveryOnLockingFailure() {
+ String mapName = "optimisticRemoveLockingFailure";
+ RedisAggregationRepository repo = createRepo(mapName, true);
+
+ try {
+ // 1. Add the original Exchange
+ Exchange original = createExchange("original");
+ repo.add(context, "key1", null, original);
+
+ // 2. Retrieve the original Exchange
+ Exchange stale = repo.get(context, "key1");
+
+ // 3. Simulate another update to the active Exchange
+ Exchange updated = createExchange("updated");
+ repo.add(context, "key1", stale, updated);
+
+ // 4. Attempt to remove the stale Exchange
+ assertThrows(OptimisticLockingException.class,
+ () -> repo.remove(context, "key1", stale));
+
+ // 5. Verify the updated Exchange remains active
+ Exchange result = repo.get(context, "key1");
+ assertNotNull(result);
+ assertEquals("updated", result.getIn().getBody(String.class));
+
+ // 6. Verify the stale Exchange was removed from recovery storage
Review Comment:
🔍 **Comment inaccuracy:** `"removed from recovery storage"` implies the
stale exchange was placed there first and then removed. But the Lua script's
CAS-failure path returns `0` without ever executing `HSET` on the recovery map
— the stale exchange is **never written** to recovery storage.
```suggestion
// 6. Verify the stale Exchange was never written to recovery
storage
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
