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

PLT-3535 MarloweBulkSync Server #724

Merged
merged 12 commits into from
Oct 5, 2023
Merged

PLT-3535 MarloweBulkSync Server #724

merged 12 commits into from
Oct 5, 2023

Conversation

jhbertra
Copy link
Contributor

@jhbertra jhbertra commented Oct 4, 2023

  • Added new bulk sync protocol.
  • Added a server implementation to marlowe-sync
  • Rewrote marlowe-finder to use bulk sync.

Pre-submit checklist:

  • Branch
    • Tests are provided (if possible)
    • Test report is updated (if relevant)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
    • Relevant tickets are mentioned in commit messages
    • Formatting, PNG optimization, etc. are updated
  • PR
    • Self-reviewed the diff
    • Useful pull request description
      • Review required
      • Substantial changes to code, test, or documentation
      • Change made to Marlowe validator (@bwbush and @palas must be included as reviewers)
      • Review not required
      • Minor changes to non-critical code, documentation, nix derivations, configuration files, or scripts
      • Formatting, spelling, grammar, or reorganization
    • Reviewer requested

@jhbertra jhbertra requested review from paluh and bwbush October 4, 2023 12:17
Comment on lines +52 to +58
syncBulkPort :: CliOption OptionFields PortNumber
syncBulkPort =
port
"marlowe-bulk"
"SYNC_MARLOWE_BULK"
3730
"The port number of the marlowe-sync server's bulk synchronization API."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Won't the operables, deployment, and compose derivations need updating, too?

In fact, should we add this reminder to the PR template?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That might be a good way to deal with this. You're right we often forget this. I can't really think of a good way to automate this check.

bwbush
bwbush previously requested changes Oct 4, 2023
Copy link
Collaborator

@bwbush bwbush left a comment

Choose a reason for hiding this comment

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

It's great to finally have this API.

However, marlowe-finder does not report about 30% of the contracts on preprod:

$ marlowe-finder --end-at-tip \
| jq -r 'select(.NewContract) | .NewContract.fields.contractId \
| (.txId + "#" + (.txIx | tostring))' \
| wc -l
5123

$ psql preprod
preprod=> select count(*) from marlowe.createtxout;
 count 
-------
  7055
(1 row)

@bwbush
Copy link
Collaborator

bwbush commented Oct 4, 2023

The situation is more serious on preview:

$ marlowe-finder --end-at-tip \
| jq -r 'select(.NewContract) | .NewContract.fields.contractId | (.txId + "#" + (.txIx | tostring))' \
| wc -l
15868

$ psql preview
preview=> select count(*) from marlowe.createtxout;
 count 
-------
 52969
(1 row)

@jhbertra
Copy link
Contributor Author

jhbertra commented Oct 4, 2023

Thanks for performing that test, I'll investigate!

@jhbertra jhbertra dismissed bwbush’s stale review October 4, 2023 18:14

Issue addressed

@jhbertra jhbertra force-pushed the plt-3535-bulk-sync branch from b504618 to 4e9906a Compare October 4, 2023 18:42
@bwbush bwbush self-requested a review October 4, 2023 22:04
Copy link
Collaborator

@bwbush bwbush left a comment

Choose a reason for hiding this comment

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

@jhbertra, I reran the tests on the new commit and results are correct both on preprod and preview.

@jhbertra jhbertra merged commit 8bd0039 into main Oct 5, 2023
@jhbertra jhbertra deleted the plt-3535-bulk-sync branch October 5, 2023 11:10
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.

2 participants