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

feat/clusterdb #11069

Merged
merged 38 commits into from
Jul 27, 2023
Merged

feat/clusterdb #11069

merged 38 commits into from
Jul 27, 2023

Conversation

snadrus
Copy link
Collaborator

@snadrus snadrus commented Jul 12, 2023

Proposed Changes
Introduces HarmonyDB, a DB adapter that

  1. handles multiple Yugabyte instances,
  2. prevents SQL injection,
  3. automatically manages version despite a non-linear commit history,
  4. allows parallel integration test runs
  5. keeps precise Prometheus metrics, and
  6. has powerful helpers (QueryRow, Select, BeginTransaction) with examples.
  7. Includes changes to integration test framework so testing is now trivial both on circleci and for locally-installed default yugabyte servers.

@snadrus snadrus requested a review from a team as a code owner July 12, 2023 02:58
@snadrus snadrus requested review from magik6k and shrenujbansal July 12, 2023 02:59
Copy link
Contributor

Choose a reason for hiding this comment

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

looks accidentally committed

Comment on lines +37 to +39
if err != nil {
t.Fatal("Could not insert: ", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We generally use testify / require.NoError(t, err);

I don't think this needs changing, but would be more consistent with the rest of the codebase

"time"

logging "github.com/ipfs/go-log/v2"
"github.com/jackc/pgx/v5"
Copy link
Contributor

Choose a reason for hiding this comment

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

Btw yugabyte has a cluster-aware fork of pgx which we could consider using - https://github.com/yugabyte/pgx

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since we have pgx v5 and they forked v4, I'm not sure what we are missing.
But between already supporting fallback and allowing SPs to set the initial server, I don't feel a big need to move to this, especially since only 1 db instance will be common at the beginning.

Comment on lines 755 to 757
// ITest is for optimized integration testing and not
// for production. Blank for default production configuration.
ITest string
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be needed

@snadrus snadrus merged commit 1d58bf0 into feat/sturdypost Jul 27, 2023
@snadrus snadrus deleted the clusterdb branch July 27, 2023 23:43
@magik6k magik6k mentioned this pull request Jul 28, 2023
@rjan90 rjan90 added this to the HA WindowPoSt milestone Aug 1, 2023
@rjan90 rjan90 linked an issue Aug 1, 2023 that may be closed by this pull request
@jennijuju jennijuju mentioned this pull request Aug 1, 2023
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.

Lotus DB package
4 participants