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.

Sign up for GitHub

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

Jump to bottom

CORE-15552 Add findUnconsumedStatesByExactType paging API #4837

Merged
merged 7 commits into from
Oct 13, 2023
Merged

CORE-15552 Add findUnconsumedStatesByExactType paging API #4837

merged 7 commits into from
Oct 13, 2023

Conversation

lankydan
Copy link
Contributor

@lankydan lankydan commented Oct 11, 2023 •
edited
Loading

Add findUnconsumedStatesByExactType to UtxoLedgerService , allowing developers to retrieve states by type from the vault in pages. This dissuades developers from retrieving every state from the vault in one go and returning them to a flow.

Reuse the vault named query logic to create a
findUnconsumedStatesByExactType API that supports paging without too much extra work.

DefaultVaultNamedQueryFactoryImpl registers a query for finding states by their exact type.

This factory is then manually registered in
VaultNamedQueryFactoryProvider.

We could potentially add a better lookup mechanism in the future, but there is unlikely to be additional VaultNamedQueryFactorys that we register in the future.

@lankydan lankydan self-assigned this Oct 11, 2023
@lankydan lankydan requested a review from a team as a code owner October 11, 2023 11:56
@lankydan
Copy link
Contributor Author

API PR - corda/corda-api#1292

relyafi
relyafi reviewed Oct 11, 2023
View reviewed changes
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.

I think the overall approach to re-using the existing custom query logic is a good idea, but I'm unconvinced about the need for a separate factory for this query.

Also, I'd like to see some new E2E test cases proving that the query works with the paging functionality. If we want to do this in the E2E test repo (rather than smoke tests in this PR) that's fine, but we should reference that PR from this one.

...nce/src/main/kotlin/net/corda/ledger/persistence/query/impl/DefaultVaultNamedQueryFactory.kt Outdated Show resolved Hide resolved
...nce/src/main/kotlin/net/corda/ledger/persistence/query/impl/DefaultVaultNamedQueryFactory.kt Outdated Show resolved Hide resolved
...otlin/net/corda/ledger/persistence/query/registration/impl/VaultNamedQueryFactoryProvider.kt Outdated Show resolved Hide resolved
...sistence/src/main/kotlin/net/corda/ledger/persistence/utxo/impl/AbstractUtxoQueryProvider.kt Outdated
Comment on lines 64 to 66
JOIN {h-schema}utxo_transaction_output AS tx_o
ON tx_o.transaction_id = tc_output.transaction_id
AND tx_o.leaf_idx = tc_output.leaf_idx
JOIN {h-schema}utxo_transaction AS tx
ON tx.transaction_id = tx_o.transaction_id
WHERE tx_o.type = :type
ON tx.id = tc_output.transaction_id
WHERE vto.type = :type
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why this is appearing in the diff. Looks like it's part of @nkovacsx recent data model simplification changes, so if we've merged / rebased these then this shouldn't be a diff?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a correction, there wasn't an E2E test around this so it failed when tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code is now deleted anyway.

@lankydan
Copy link
Contributor Author

E2E tests - https://github.com/corda/corda-e2e-tests/pull/276

@lankydan lankydan force-pushed the dan/CORE-15552-unconsumed-states-by-type-paging branch from de9a4d2 to 85df66f Compare October 11, 2023 13:35
@lankydan lankydan requested a review from a team as a code owner October 11, 2023 13:35
@corda-jenkins-ci02
Copy link
Contributor

corda-jenkins-ci02 bot commented Oct 11, 2023 •
edited
Loading

Jenkins build for PR 4837 build 17

Build Successful:
Jar artifact version produced by this PR: 5.1.0.0-alpha-1697210261413
Helm chart version produced by this PR: 5.1.0-alpha.1697210261413
Helm chart pushed to: oci://corda-os-docker-dev.software.r3.com/helm-charts/pr-4837/corda

vlajos
vlajos previously approved these changes Oct 13, 2023
View reviewed changes
Copy link
Contributor

@vlajos vlajos left a comment

Choose a reason for hiding this comment

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

It needs a sync, but otherwise, LGTM.

@lankydan lankydan force-pushed the dan/CORE-15552-unconsumed-states-by-type-paging branch 2 times, most recently from cd03e19 to 24bf58e Compare October 13, 2023 11:23
@lankydan lankydan requested review from a team as code owners October 13, 2023 11:23
@lankydan lankydan requested a review from a team October 13, 2023 11:23
@lankydan lankydan requested a review from a team as a code owner October 13, 2023 11:23
@lankydan lankydan force-pushed the dan/CORE-15552-unconsumed-states-by-type-paging branch from 24bf58e to 2669741 Compare October 13, 2023 11:51
@lankydan lankydan removed request for a team October 13, 2023 11:52
@lankydan lankydan removed the request for review from a team October 13, 2023 11:52
vlajos
vlajos previously approved these changes Oct 13, 2023
View reviewed changes
@lankydan lankydan force-pushed the dan/CORE-15552-unconsumed-states-by-type-paging branch from 0a9d470 to df46ed8 Compare October 13, 2023 13:21
@blsemo blsemo added the 5.1 label Oct 13, 2023
@lankydan
CORE-15552 Add findUnconsumedStatesByExactType paging API
685376d 
Reuse the vault named query logic to create a
`findUnconsumedStatesByExactType` API that supports paging without too
much extra work.

`DefaultVaultNamedQueryFactoryImpl` registers a query for finding states
by their exact type.

This factory is then manually registered in
`VaultNamedQueryFactoryProvider`.

We could potentially add a better lookup mechanism in the future, but
there is unlikely to be additional `VaultNamedQueryFactory`s that we
register in the future.
@lankydan
CORE-15552 PR comments
b542aa4
@lankydan
CORE-15552 PR comments
5151214
@lankydan
CORE-15552 Remove non paged api
1bc77a3
@lankydan
CORE-15552 Remove non paged api
c52d38d
@lankydan
CORE-15552 Compilation
ca53d9a
@lankydan
CORE-15552 Version
26dd2a4
@lankydan lankydan force-pushed the dan/CORE-15552-unconsumed-states-by-type-paging branch from 2534fe8 to 26dd2a4 Compare October 13, 2023 14:32
blsemo
blsemo approved these changes Oct 13, 2023
View reviewed changes
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

@lankydan lankydan merged commit b3b40d3 into release/os/5.1 Oct 13, 2023
1 check passed
@lankydan lankydan deleted the dan/CORE-15552-unconsumed-states-by-type-paging branch October 13, 2023 16:06
@corda-jenkins-ci02 corda-jenkins-ci02 bot mentioned this pull request Oct 18, 2023
Merged
@corda-jenkins-ci02 corda-jenkins-ci02 bot mentioned this pull request Oct 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@relyafi relyafi relyafi left review comments

@vlajos vlajos vlajos approved these changes

@blsemo blsemo blsemo approved these changes

Assignees

@lankydan lankydan

Labels
5.1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lankydan @blsemo @vlajos @relyafi