Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CORE-16183 Ledger data model simplifications (part 1) #1270

Merged

Conversation

nkovacsx
Copy link
Contributor

@nkovacsx nkovacsx commented Sep 28, 2023

Overview

This PR contains the AGREED suggestions from this page: https://r3-cev.atlassian.net/wiki/spaces/CB/pages/4578214028/UTXO+Ledger+Data+Model+Simplifications#Remove-created-field-from-utxo_transaction_component-table-Agreed.

These are:

  • Merge utxo_transaction_status table into utxo_transaction (then delete the utxo_transaction_status table) and rename utxo_transaction_output table to utxo_visible_transaction_output
  • Merge utxo_visible_transaction_state table into utxo_transaction_output (then delete the utxo_visible_transaction_state table)
  • Removal of utxo_transaction_sources table
  • Removal of utxo_transaction_cpk and utxo_cpk tables
  • Remove created field from utxo_transaction_component table

Test plan

  1. 5.0 -> 5.1
    The upgrade SQL file was generated using the Corda CLI tool with the latest plugin. The SQL generated then was executed in a live Postgres database. The existing transactions were moved according to the 5.1 schema accordingly.

  2. 5.1 Postgres
    Started up a 5.1 instance with a fresh Postgres instance and schema creation ran successfully.

@corda-jenkins-ci02
Copy link
Contributor

corda-jenkins-ci02 bot commented Sep 28, 2023

Jenkins build for PR 1270 build 11

Build Successful:
Jar artifact version produced by this PR: 5.1.0.30-alpha-1696856496444

@nkovacsx nkovacsx requested a review from vlajos October 3, 2023 08:48
vlajos
vlajos previously approved these changes Oct 3, 2023
Copy link
Contributor

@relyafi relyafi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments but broadly looks good, final approval should be deferred until corresponding corda-runtime-os changes are also ready for review and approved.

I think it might be worth clarifying the test scenarios in the test notes, what I'd like to see is:

  • Able to deploy a "fresh" Postgres based Corda deployment
  • Able to run up a "fresh" simulator deployment (or the combined worker) with HSQLDB. I'm assuming no upgrade concerns with HSQLDB as we'd always use a fresh deployment given its in-memory nature
  • Able to deploy a Postgres based Corda deployment running 5.0, generate some transactions, and then upgrade to 5.1. Ideally we'd have an integration test for this in runtime-os (though not sure if our test framework is mature enough to do this yet)

…nto nandor/CORE-16183/ledger-data-model-simplifications
…nto nandor/CORE-16183/ledger-data-model-simplifications
Copy link
Collaborator

@lankydan lankydan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes generally look fine to me, left one comment on another comment though.

One other thing

These are:

Merge utxo_transaction_status table into utxo_transaction (then delete the utxo_transaction_status table)
Merge utxo_visible_transaction_state table into utxo_transaction_output (then delete the utxo_visible_transaction_state table)
Removal of utxo_transaction_sources table
Removal of utxo_transaction_cpk and utxo_cpk tables
Remove created field from utxo_transaction_component table
Rename utxo_transaction_output table to utxo_visible_transaction_output

Can we either specify these as steps or only say what the outcome is.

Mainly seeing "Merge utxo_visible_transaction_state table into utxo_transaction_output" and then "utxo_transaction_output table to utxo_visible_transaction_output" confused me when reading through the change.

If it was "Merge utxo_visible_transaction_state table into utxo_transaction_output and rename it to utxo_visible_transaction_output" it might be clearer. This is nit-picky though.

…nto nandor/CORE-16183/ledger-data-model-simplifications

# Conflicts:
#	data/db-schema/src/main/resources/net/corda/db/schema/vnode-vault/migration/ledger-utxo-creation-v5.1.xml
vlajos
vlajos previously approved these changes Oct 9, 2023
lankydan
lankydan previously approved these changes Oct 9, 2023
relyafi
relyafi previously approved these changes Oct 9, 2023
@nkovacsx nkovacsx requested a review from blsemo October 9, 2023 13:00
@nkovacsx nkovacsx dismissed stale reviews from relyafi, lankydan, and vlajos via 614c37d October 9, 2023 13:01
Copy link
Contributor

@blsemo blsemo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nkovacsx nkovacsx merged commit 931648d into release/os/5.1 Oct 9, 2023
5 checks passed
@nkovacsx nkovacsx deleted the nandor/CORE-16183/ledger-data-model-simplifications branch October 9, 2023 13:16
nkovacsx added a commit to corda/corda-runtime-os that referenced this pull request Oct 9, 2023
…s (part 1) (#4754)

These are the runtime-os side changes of the data model simplifications from corda/corda-api#1270.
- Merge utxo_transaction_status table into utxo_transaction (then delete the utxo_transaction_status table) and rename utxo_transaction_output table to utxo_visible_transaction_output
- Merge utxo_visible_transaction_state table into utxo_transaction_output (then delete the utxo_visible_transaction_state table)
- Removal of utxo_transaction_sources table
- Removal of utxo_transaction_cpk and utxo_cpk tables
- Remove created field from utxo_transaction_component table
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants