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

SQL Database Interfaces Definition #2659

Merged
merged 4 commits into from
Jul 18, 2023

Conversation

em-r
Copy link
Contributor

@em-r em-r commented Jul 11, 2023

What type of PR is this?

This PR introduces the definition of interfaces proposed in this design https://github.com/armadaproject/armada/blob/master/docs/design/database_interfaces.md

What this PR does / why we need it:

Those interfaces are meant to establish an abstraction over the existing Postgres implementation which will eventually lead to removing the tight coupling between Armada and Postgres.

Special notes for your reviewer:

This is a follow PR to #2624, and it's meant particularly to introduce the interfaces that were proposed in the design. Follow PRs will be raised regarding the implementation of the interfaces as well as integrating those implementations with Armada.

┆Issue is synchronized with this Jira Task by Unito

@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (4d24856) 16.21% compared to head (3f5cdd7) 16.21%.

❗ Current head 3f5cdd7 differs from pull request most recent head e4a0844. Consider uploading reports for the commit e4a0844 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2659   +/-   ##
=======================================
  Coverage   16.21%   16.21%           
=======================================
  Files         160      160           
  Lines       12559    12559           
  Branches      487      487           
=======================================
  Hits         2037     2037           
  Misses      10353    10353           
  Partials      169      169           
Flag Coverage Δ
unittests 16.21% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@kannon92 kannon92 self-assigned this Jul 11, 2023
@kannon92 kannon92 self-requested a review July 11, 2023 11:34
@kannon92
Copy link
Contributor

Just did a glance, it looks good to me. Can you fix the lint issues?

@Mo-Fatah Mo-Fatah self-requested a review July 18, 2023 15:18
@kannon92
Copy link
Contributor

@Mo-Fatah

@kannon92 kannon92 enabled auto-merge (squash) July 18, 2023 16:16
@kannon92 kannon92 merged commit 4142a50 into armadaproject:master Jul 18, 2023
21 of 23 checks passed
Copy link
Collaborator

@Mo-Fatah Mo-Fatah 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 this looks great! I added a few comments that you can consider in another PR since this is already merged. Let me know what you think!


// BeginTxFunc starts a transaction and executes the given function within the transaction.
// It the function runs successfully, BeginTxFunc commits the transaction, otherwise it rolls back and return an error.
BeginTxFunc(context.Context, DatabaseTxOptions, func(DatabaseTx) error) error
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this necessary ? BeginTxFunc is no longer supported in pgx/v5 . see jackc/pgx#1169 (comment) .
This won't make the interface compatible with pgx/v5 . I think we should remove it.

BeginTx(context.Context, DatabaseTxOptions) (DatabaseTx, error)

// BeginTxFunc starts a transaction and executes the given function within the transaction. It the function runs successfully, BeginTxFunc commits the transaction, otherwise it rolls back and return an errorr.
BeginTxFunc(context.Context, DatabaseTxOptions, func(DatabaseTx) error) error
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same


// Scan reads the values from the current row into dest values positionally. It returns an error if any occurred during the read operation.
Scan(dest ...any) error
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should add another interface that acts as a generic placeholder for both DatabasePool and DatabaseConn . In various parts of the armada, such as here , the Querier interface is a placeholder for both pgxpool.Pool pgx.Conn. It is a loose interface that requires only 3 functions https://pkg.go.dev/github.com/jackc/pgtype@v1.13.0/pgxtype#Querier

You can introduce a new similar one here for DatabasePool and DatabaseConn that requires maybe only Exec and Query .

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.

None yet

4 participants