Extract IdempotencyRecord data-model change and JDBC schema v5#4431
Extract IdempotencyRecord data-model change and JDBC schema v5#4431huaxingao wants to merge 1 commit into
Conversation
github-project-automation
Bot
moved this from PRs In Progress
to Done
in Basic Kanban Board
github-project-automation
Bot
moved this from Done
to PRs In Progress
in Basic Kanban Board
|
@dimas-b — this is the prerequisite PR split out of #4269 per your suggestion. It isolates the IdempotencyRecord data-model change and the JDBC schema v5 bump so they can be reviewed independently from the handler-level integration work. Once this lands I'll rebase #4269 on top. PTAL when you have a moment. |
|
also cc @flyrain @singhpk234 |
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
No concerns from my side, but I'm not very familiar with the Idempotency impl. internals to approve 😅
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
Changes LGTM at high level.
Approving to unblock progress. I suppose merging on Mon is reasonable unless new review comments are posted.
| idempotency_key TEXT NOT NULL, | ||
| operation_type TEXT NOT NULL, | ||
| resource_id TEXT NOT NULL, -- normalized request-derived resource identifier (not a generated entity id) | ||
| principal_hash TEXT NOT NULL, -- hash of caller principal + realm; checked on replay to prevent cross-principal cache hits |
There was a problem hiding this comment.
I think it's fine to merge now, but actual implementation probably needs a more in-depth discussion. Do we have a doc/email about this that I missed 🤔
github-project-automation
Bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
singhpk234
left a comment
singhpk234
left a comment
There was a problem hiding this comment.
LGTM too, thanks @huaxingao
| Thread.currentThread() | ||
| .getContextClassLoader() | ||
| .getResourceAsStream("postgres/schema-v4.sql")) { | ||
| .getResourceAsStream("postgres/schema-v5.sql")) { |
There was a problem hiding this comment.
orthogonal: we should make sure it run with all version shipped
| // Hash of caller principal identity bound to the reservation. | ||
| // Compared on replay to prevent cross-principal cache hits. |
There was a problem hiding this comment.
i would rather just say authenticated {@PolarisPrincipal} associated with the idempotency record ... something.
| // Timestamp when the record was created. | ||
| String CREATED_AT = "created_at"; | ||
| // Timestamp when the record was last updated. | ||
| String UPDATED_AT = "updated_at"; | ||
| // Timestamp for the last heartbeat update (null if no heartbeat recorded). | ||
| String HEARTBEAT_AT = "heartbeat_at"; | ||
| // Identifier of the executor that owns the in-progress record (null if not owned). | ||
| String EXECUTOR_ID = "executor_id"; | ||
| // Expiration timestamp after which the record can be considered stale/purgeable. | ||
| String EXPIRES_AT = "expires_at"; |
There was a problem hiding this comment.
why are we removing this comments ?
There was a problem hiding this comment.
I do not mind keeping or deleting these comments, but IMHO the value of these comments is pretty low: their meaning is deducible from field names.
Specifying behaviour here is not the ideal place, because there's no logic in this code area. Logic for processing the values for these columns exists somewhere else.
There was a problem hiding this comment.
Agree with @dimas-b that these comments are not quite valuable given the field names themselves are descriptive already. +1 on removing them.
flyrain
left a comment
flyrain
left a comment
There was a problem hiding this comment.
LGTM. Thanks @huaxingao !
| // Timestamp when the record was created. | ||
| String CREATED_AT = "created_at"; | ||
| // Timestamp when the record was last updated. | ||
| String UPDATED_AT = "updated_at"; | ||
| // Timestamp for the last heartbeat update (null if no heartbeat recorded). | ||
| String HEARTBEAT_AT = "heartbeat_at"; | ||
| // Identifier of the executor that owns the in-progress record (null if not owned). | ||
| String EXECUTOR_ID = "executor_id"; | ||
| // Expiration timestamp after which the record can be considered stale/purgeable. | ||
| String EXPIRES_AT = "expires_at"; |
There was a problem hiding this comment.
Agree with @dimas-b that these comments are not quite valuable given the field names themselves are descriptive already. +1 on removing them.
|
Is this PR superseded by #4659 ? |
|
Superseded by #4659 |
github-project-automation
Bot
moved this from Ready to merge
to Done
in Basic Kanban Board
4 participants
Prerequisite split out of #4269 per review feedback.
IdempotencyRecord: addprincipalHash, drop unusedresponseHeaders,remove redundant
@Overrideaccessors.IdempotencyStore.finalizeRecord: drop theresponseHeadersparameter.principal_hash(NOT NULL) and dropresponse_headerson
idempotency_records. Bumps schema version 4 → 5.Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)