idempotent rds module added#37
Conversation
630d40f to
29151fa
Compare
There was a problem hiding this comment.
I don’t fully understand the flow with the silent fail. This approach requires special handling in each store implementation rather than being managed at the core level. For example, if the key already exists during store(), the request should be treated as pending and wait for the result of the previous request that started executing the business logic. This way, in the case of concurrent requests, two requests with the same key are not treated as new requests.
In #29 there is a different approach. Is it better to close and merge that PR, or to completely change the handling?
|
@burl21 - Just addressing your last 3 comments, let me know if it makes sense: RDS: We use conditional upserts (ON CONFLICT DO UPDATE WHERE ...) to handle atomic expired-lock recovery. A strict insert would force an unsafe read-then-mutate cycle, creating a race condition where two workers could both think they've "claimed" an expired lock. NATS: The CAS (revision) is essential because it guarantees that an update() only succeeds if we still own the exact lock version we acquired. This prevents us from accidentally overwriting a key if our own lock had expired and was already re-claimed by another worker. Redis Lua : Standard Redis commands (like SETNX) can't inspect the internal state (status/expiration) of a JSON payload. Lua allows us to perform that check and the subsequent overwrite as a single atomic transaction, ensuring we don't have interleaved writes. Clock Skew (TODO Item in a follow up PR): We currently rely on JVM time. In a future iteration, we should move to native storage time (e.g., current_timestamp, DB TIME) to completely eliminate the risk of distributed clock skew between app nodes. |
Qodana for JVM1 new problem were found
View the detailed Qodana reportTo be able to view the detailed Qodana report, you can either:
To get - name: 'Qodana Scan'
uses: JetBrains/qodana-action@v2025.3.1
with:
upload-result: trueContact Qodana teamContact us at qodana-support@jetbrains.com
|
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
|
However, this does not solve the issue that the core must handle the different scenarios. Currently, the logic has been moved into the store implementations, which in my opinion is incorrect because it does not cover the case of concurrent requests. If store() does not perform a strict insert and, for operations by same key, the entry already exists, the core will treat the second insert as a newRequest and execute the business logic multiple times, which must not happen. For NATS, I believe it is redundant to perform two operations (get + update). In the case of a large response, this adds unnecessary latency. In my view, the core should not allow a request to perform an update; that would indicate a bug, because two requests with the same key would be treated as newRequest. For Redis, I do not see a valid reason to inspect the body since a TTL is already in place. Moreover, this implies that the Lua script must be enabled, and unfortunately many environments disable Lua for security and/or performance reasons (as it can block the Redis server). Example: IdempotentStore.Value value =
idempotentStore.getValue(idempotentKey, ((MethodSignature) pjp.getSignature()).getReturnType());
if (isExistingRequest(value)) {
log.atDebug().log("Idempotent key {} already exists", key);
return handleExistingRequest(pjp, idempotentKey, value, ttl);
}
log.atDebug().log("Idempotent key {} does not exist, creating new entry", key);
return handleNewRequest(pjp, idempotentKey, ttl); // <--- should manage existing requestsWithout a strict insert, handleNewRequest will execute the business logic multiple times instead of waiting for the response to be completed. I would also add that these points should most likely be addressed in a new PR, so that we can decide which direction to take, as mentioned in the other PR. |
36f8524 to
78f0f26
Compare
|
Thank you for the detailed and insightful feedback. I completely agree with your assessment. I've taken the following actions in the latest commits to align with your suggestions: Reverted Concurrency Logic: I have reverted all changes to idempotent-core, idempotent-nats, idempotent-redis, and idempotent-dynamo. These modules are now back to their main branch state, removing the experimental atomic checks. Simplified RDS Implementation: The new RdsIdempotentStore has been simplified to perform straightforward UPSERTs, matching the behavior and simplicity of the existing stores. |
78f0f26 to
15216df
Compare
|
I’ve added a few comments, mainly regarding RdsConfig. Everything else looks good to me. |
Also:
|
44659a7 to
e012fcb
Compare
e012fcb to
aa280f1
Compare
|
For the configuration-processor to generate the property description, the parameters must be documented in the Javadoc, I suggest changing /**
* Configuration properties for RDS-based idempotency implementation.
* <p>
* These properties control the behavior of the idempotent key storage and cleanup
* in a relational database system.
* </p>
*
* @param tableName the name of the database table used to store idempotent keys
* @param cleanup configuration for the cleanup task that removes expired idempotent records
*/
@ConfigurationProperties(prefix = "idempotent.rds")
public record RdsIdempotentProperties(
@DefaultValue("idempotent") String tableName, Cleanup cleanup) {
/**
* Configuration properties for the cleanup task that removes expired idempotent records.
* <p>
* The cleanup task runs periodically to delete expired idempotent keys from the database,
* preventing unbounded growth of the idempotent key table.
* </p>
*
* @param enabled whether the cleanup task should run
* @param batchSize maximum number of expired records to delete in a single cleanup operation
* @param fixedDelay delay in milliseconds between consecutive cleanup task executions
*/
public record Cleanup(
@DefaultValue("true") boolean enabled,
@DefaultValue("1000") int batchSize,
@DefaultValue("60000") long fixedDelay) {}
} |
|
Summary: Adds a JDBC-based storage implementation for idempotency keys using JdbcTemplate.
Key Features:
This PR resolves: #6
cc: @maciejwalkowiak