CORE-13621 Idempotency id table definition #1222
Conversation
kyriathar
force-pushed
the
kyriakos/CORE-13621-idemopotency-id-table
branch
from
5e4f712 to
a54e444
Compare
|
Jenkins build for PR 1222 build 9 Build Successful: |
kyriathar
force-pushed
the
kyriakos/CORE-13621-idemopotency-id-table
branch
from
a54e444 to
61f4833
Compare
kyriathar
force-pushed
the
kyriakos/CORE-13621-idemopotency-id-table
branch
from
61f4833 to
d6c774e
Compare
kyriathar
changed the title
kyriathar
force-pushed
the
kyriakos/CORE-13621-idemopotency-id-table
branch
2 times, most recently
from
f345126 to
ad613b3
Compare
| <column name="request_id" type="NVARCHAR(255)"> | ||
| <constraints nullable="false" primaryKey="true"/> | ||
| </column> | ||
| </createTable> |
There was a problem hiding this comment.
What is the cleanup strategy for this table?
Would it not be useful to have a timestamp column that can be used for partitioning and/or cleanup?
Given it is insert only, I suspect it would be very cheap to have a DB auto-populated timestamp column, but we could check with @ifrankrui
There was a problem hiding this comment.
The cleanup strategy will be done in follow-up work. This PR only introduces the part of the deduplication table (the request ids) needed to deduplicate the persistence requests (in corda/corda-runtime-os#4491).
There was a problem hiding this comment.
But do we have a design yet for the clean-up strategy?
I'm a bit uncomfortable with merging something in which we know will cause problems, without a plan for how we will mitigate it.
And if we know what the design will be, we may as well get the DB structure correct from the get-go.
There was a problem hiding this comment.
Don't you already include the housekeeping strategy here?
Yes, you could have a timestamp column for the deduplication table and populate the timestamp as a part of the insert request. However, I would not recommend using DB auto-populated timestamp column. They are implemented differently between databases and there isn't one simple way to define them in Liquibase change log.
There was a problem hiding this comment.
there isn't one simple way to define them in Liquibase change log.
@ifrankrui I'm interested in this comment. Why is that?
We do this in a number of places already (e.g.:
There was a problem hiding this comment.
@driessamyn As pointed out in the above changelog, you have to define an extra property for one type of database when you support multiple database types. If you populate the timestamp column as a part of a persistence request, then you don't need these Liquibase properties.
kyriathar
force-pushed
the
kyriakos/CORE-13621-idemopotency-id-table
branch
from
ad613b3 to
77afe26
Compare
kyriathar
force-pushed
the
kyriakos/CORE-13621-idemopotency-id-table
branch
from
77afe26 to
f3c1624
Compare
| @@ -6,5 +6,11 @@ | |||
| <addColumn tableName="vnode_member_info"> | |||
| <column name="is_deleted" type="BOOLEAN" defaultValueBoolean="false"/> | |||
| </addColumn> | |||
|
|
|||
| <createTable tableName="vnode_persistence_request_id"> | |||
| <column name="request_id" type="NVARCHAR(255)"> | |||
There was a problem hiding this comment.
Just passing by, but I wonder if NVARCHAR(36) would be suffice as I think a UUID fits in it? I guess it depends how we build the idempotency ID.
There was a problem hiding this comment.
We only store the UUID.toString() in it. Will take a look to see if we can shorten the DB field.
Introduces table in vnode-vault schema which holds executed persistence requests (ids) from
PersistenceService, to be used for deduplicating them.runtime-os PR: #4491