Idempotency: JDBC store implementation and SPI#3915
Idempotency: JDBC store implementation and SPI#3915huaxingao wants to merge 5 commits intoapache:mainfrom
Conversation
Introduce/extend the IdempotencyStore SPI and implement it for relational-jdbc, including tests. Made-with: Cursor
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
Thanks for your continuous work on this feature, @huaxingao ! Sorry about the delayed review.
polaris-core/src/main/java/org/apache/polaris/core/persistence/IdempotencyStoreFactory.java
Outdated
Show resolved
Hide resolved
polaris-core/src/main/java/org/apache/polaris/core/entity/IdempotencyRecord.java
Outdated
Show resolved
Hide resolved
...e/polaris/persistence/relational/jdbc/idempotency/RelationalJdbcIdempotencyStoreFactory.java
Outdated
Show resolved
Hide resolved
polaris-core/src/main/java/org/apache/polaris/core/persistence/IdempotencyStore.java
Outdated
Show resolved
Hide resolved
polaris-core/src/main/java/org/apache/polaris/core/entity/IdempotencyRecord.java
Outdated
Show resolved
Hide resolved
|
@dimas-b Thanks for your review! I have addressed you comments. Could you please take a look again when you have time? |
|
also cc @singhpk234 @flyrain @flyingImer |
| String errorSubtype = rs.getString(ERROR_SUBTYPE); | ||
| String responseSummary = rs.getString(RESPONSE_SUMMARY); | ||
| String responseHeaders = rs.getString(RESPONSE_HEADERS); | ||
| Map<String, List<String>> responseHeaders = null; |
There was a problem hiding this comment.
Is Polaris is expected to have any old data for this value?
There was a problem hiding this comment.
No, I don't think so.
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
| Set.of()); | ||
| datasourceOperations.executeUpdate(delete); | ||
| } catch (SQLException e) { | ||
| throw new IdempotencyPersistenceException( |
There was a problem hiding this comment.
I think there are two high-priority issues worth fixing before this moves forward. First, the Postgres IT still uses Map.of("Content-Type", "application/json") even though finalizeRecord() now expects Map<String, List<String>>, which looks like a straight compile/type mismatch. Second, the SPI docs describe cancelInProgressReservation() as best-effort and explicitly say failures should not affect the response path, but the JDBC impl now throws on cancel failure. Would it make sense to either make the JDBC path log-and-swallow here, or tighten the SPI contract + callers so the boundary is explicit?
3 participants
This PR introduces the persistence SPI for idempotency records and adds a relational-JDBC implementation.
This is split from #3803
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)