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

Support rollback nodes in TransactionIndexing #9506

Merged
merged 3 commits into from
Apr 27, 2021
Merged

Support rollback nodes in TransactionIndexing #9506

merged 3 commits into from
Apr 27, 2021

Conversation

cocreature
Copy link
Contributor

changelog_begin
changelog_end

Pull Request Checklist

  • Read and understand the contribution guidelines
  • Include appropriate tests
  • Set a descriptive title and thorough description
  • Add a reference to the issue this PR will solve, if appropriate
  • Include changelog additions in one or more commit message bodies between the CHANGELOG_BEGIN and CHANGELOG_END tags
  • Normal production system change, include purpose of change in description

NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with /AzurePipelines run to
trigger the build.

@cocreature cocreature force-pushed the rollback-filter-indexer branch 2 times, most recently from bda2f41 to 6543b37 Compare April 27, 2021 09:59
tudor-da
tudor-da reviewed Apr 27, 2021
View reviewed changes
...integration-api/src/test/suite/scala/platform/store/dao/events/TransactionIndexingSpec.scala Outdated
val party = Party.assertFromString(partyStr)
val bobStr = "Bob"
val bob = Party.assertFromString(bobStr)
// val offset = LedgerOffset.Absolute(LedgerString.assertFromString("offset"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove this commented line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

6dff0ee

@tudor-da
Copy link
Contributor

This implementation targets the legacy index schema, while we are already working on the new schema (in package appendonlydao. I am not sure if it should be you to implement it in the upcoming schema. But if we decide to port it later, we need to keep track of it. @mziolekda @nmarton-da what do you think?

@cocreature
Copy link
Contributor Author

This implementation targets the legacy index schema, while we are already working on the new schema (in package appendonlydao. I am not sure if it should be you to implement it in the upcoming schema. But if we decide to port it later, we need to keep track of it. @mziolekda @nmarton-da what do you think?

Where is the new schema code for this? Contrary to some other files, this only seems to exist once. We need exception support everywhere so this absolutely needs to be ported to the new schema.

I care less about who has to do the work and more that we get it working so happy to take a shot or help someone if you can point me to the right place.

I think it probably makes sense to keep that for a separate PR though.

@cocreature cocreature force-pushed the rollback-filter-indexer branch from 6dff0ee to 6fed11d Compare April 27, 2021 12:36
@cocreature
Support rollback nodes in TransactionIndexing
bf47a7d 
changelog_begin
changelog_end
@cocreature
Remove leftover comment
86486b9 
changelog_begin
changelog_end
@cocreature
add missing dep
bd3307c 
changelog_begin
changelog_end
@cocreature cocreature force-pushed the rollback-filter-indexer branch from 6fed11d to bd3307c Compare April 27, 2021 12:48
@tudor-da
Copy link
Contributor

This implementation targets the legacy index schema, while we are already working on the new schema (in package appendonlydao. I am not sure if it should be you to implement it in the upcoming schema. But if we decide to port it later, we need to keep track of it. @mziolekda @nmarton-da what do you think?

Where is the new schema code for this? Contrary to some other files, this only seems to exist once. We need exception support everywhere so this absolutely needs to be ported to the new schema.

I care less about who has to do the work and more that we get it working so happy to take a shot or help someone if you can point me to the right place.

I think it probably makes sense to keep that for a separate PR though.

Ok, now it is more clear. It seems the new code would be already ignoring Rollback nodes. So nothing is needed in that area, I believe. Thanks, @moritzkiefer-da

@cocreature
Copy link
Contributor Author

Ok, now it is more clear. It seems the new code would be already ignoring Rollback nodes. So nothing is needed in that area, I believe. Thanks, @moritzkiefer-da

Looking at this, I’m not entirely sure that’s right.I think you filter out rollback nodes but you will still get a create under a rollback node. I’ll make sure to look into this and either fix it myself or reach out to you if I need help.

@cocreature cocreature merged commit 8747b3d into main Apr 27, 2021
@cocreature cocreature deleted the rollback-filter-indexer branch April 27, 2021 13:47
azure-pipelines bot pushed a commit that referenced this pull request Apr 28, 2021
release 1.13.0-snapshot.20210428.6782.0.e1e878a5
a9ed266 
This PR has been created by a script, which is not very smart
and does not have all the context. Please do double-check that
the version prefix is correct before merging.

@nickchapman-da is in charge of this release.

Commit log:
```
e1e878a Simplify opt-in/out of Oracle when building (#9515)
d761853 KVL-874 Add unit tests for Telemetry (#9500)
c567680 Support rollback nodes in PostCommitValidation (#9501)
a335ee8 Support rollback nodes in KeyValueCommitting.submissionOutputs (#9512)
8747b3d Support rollback nodes in TransactionIndexing (#9506)
c4cf3c9 kvutils: Log a missing input state warning without the stack trace. (#9513)
4e712a0 add oracle option for json-api perf runner (#9492)
8cd3658 Switch to an environment variable for enabling Oracle tests. (#9511)
e39c20e update GPG public key (#9488)
782109d update LATEST (#9508)
3e66611 Nest stakeholders in contracts table as JSON arrays (#9484)
aecdc2a update NOTICES file (#9507)
```
Changelog:
```
- [JSON-API Perf] ``--query-store-index=postgres`` must be passed
  to select PostgreSQL query store performance testing; ``true``
  and ``yes`` are no longer supported.
  See `issue #9492 <https://github.com/digital-asset/daml/pull/9492>`__.
```

CHANGELOG_BEGIN
CHANGELOG_END
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tudor-da tudor-da tudor-da approved these changes

@nickchapman-da nickchapman-da Awaiting requested review from nickchapman-da

@sofiafaro-da sofiafaro-da Awaiting requested review from sofiafaro-da

@gerolf-da gerolf-da Awaiting requested review from gerolf-da

@mziolekda mziolekda Awaiting requested review from mziolekda

@nmarton-da nmarton-da Awaiting requested review from nmarton-da

@rautenrieth-da rautenrieth-da Awaiting requested review from rautenrieth-da

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cocreature @tudor-da