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

Add multi-db sandbox-on-x conformance tests [DPP-802] #12585

Merged

Conversation

nmarton-da
Copy link
Contributor

  • Enhances SandboxOnXRunner with manipulateConfig
  • Adds Postgres and Oracle conformance test runner binaries
  • Adapts BUILD.bazel

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
  • If you mean to change the status of a component, please make sure you keep the Component Status page up to date.

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.

@nmarton-da nmarton-da requested a review from tudor-da January 26, 2022 00:00
@nmarton-da nmarton-da requested a review from a team as a code owner January 26, 2022 00:00
@tudor-da
Copy link
Contributor

@nmarton-da Oracle tests are timeouting

@nmarton-da nmarton-da force-pushed the dpp-802-add-multi-db-sandbox-on-x-conformance-tests branch from 1374d6e to 6a4344f Compare January 26, 2022 08:42
@nmarton-da nmarton-da requested a review from a team as a code owner January 26, 2022 08:42
main_class = "com.daml.ledger.sandbox.MainWithEphemeralPostgresql",
visibility = ["//visibility:public"],
runtime_deps = [
"@maven//:com_oracle_database_jdbc_ojdbc8",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is Oracle driver needed for Postgres?

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 definitely does not 👍
funny thing we do not need this for oracle neither: these deps are creeping in as build deps already, removed that as well

@nmarton-da nmarton-da force-pushed the dpp-802-add-multi-db-sandbox-on-x-conformance-tests branch from 6a4344f to 6d5a608 Compare January 26, 2022 11:40
@mziolekda
Copy link
Contributor

mziolekda commented Jan 27, 2022

Why don't we get rid of the corresponding tests in ledger-on-sql at the same time?
Scratch that, the ledger-on-sql project now just tests itself

@nmarton-da nmarton-da force-pushed the dpp-802-add-multi-db-sandbox-on-x-conformance-tests branch from 6d5a608 to 60b6801 Compare February 1, 2022 17:07
@nmarton-da
Copy link
Contributor Author

@meiersi-da @tudor-da @mziolekda
I dug a little deeper why this is flaky for Oracle, and it is failing due to contraint violation as inserting the same packages to the Oracle DB. This is a problem because several conditions together:

  • Currently sandbox-on-x does not support indexer restart
  • We still have a known bug for Oracle (package idempotent insert fails with unique constraint violation if concurrent db transactions racing to insert the same packages)
  • sandbox-on-x is not doing "package deduplication" -- in case ledger-on-sql, as package_upload updates arrive via ReadService, they are already not containing packages which should be already known

Since we already have a story for the first and second (and actually only one of them would fix the issue here), I propose to put this on hold until either of those are done, which will solve the flakiness here.

Regarding the last point ("package deduplication") just wanted to make you aware of this difference between sandbox-on-x and kv ledgers (or at least ledger-on-sql): not sure what is expected here. FYI since in canton Oracle tests we observed similar errors, I guess the canton ledger is also not doing "package deduplication".

@meiersi-da
Copy link
Contributor

Thanks @nmarton-da . Let's look at how to index package and .dar uploads together with @andreaslochbihler-da and @rgugliel-da as part of the storage architecture revision. We definitely want to store the package contents at most once, as they can be large. However, we might want index them being made available for interpretation multiple times.

* Enhances SandboxOnXRunner with manipulateConfig
* Adds Postgres and Oracle conformance test runner binaries
* Adapts BUILD.bazel

changelog_begin
changelog_end
@nmarton-da nmarton-da force-pushed the dpp-802-add-multi-db-sandbox-on-x-conformance-tests branch from 60b6801 to ecdef63 Compare February 4, 2022 11:41
@nmarton-da nmarton-da merged commit 5a2dffd into main Feb 4, 2022
@nmarton-da nmarton-da deleted the dpp-802-add-multi-db-sandbox-on-x-conformance-tests branch February 4, 2022 14:28
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.

5 participants