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

removes RPC and make use of HTTP API #28

Merged
merged 1 commit into from
Dec 14, 2023
Merged

removes RPC and make use of HTTP API #28

merged 1 commit into from
Dec 14, 2023

Conversation

brunocalza
Copy link
Contributor

@brunocalza brunocalza commented Dec 13, 2023

This PR removes the RPC protocol in favor of the HTTP API.

Lots of code removal, but in essence most of the code is the implementation of an HTTP client that follows this interface

type BasinProvider interface {
	CreateVault(context.Context, CreateVaultParams) error
	ListVaults(context.Context, ListVaultsParams) ([]Vault, error)
	ListVaultEvents(context.Context, ListVaultEventsParams) ([]EventInfo, error)
	WriteVaultEvent(context.Context, WriteVaultEventParams) error
}

Each one of the commands, calls a method.

Note: This PR changes some of the names in the code to be aligned with new terminology, but not all (you can expect for name refactoring in the next PRs). Also, the commands are still the same. In a future PR, I'll rename them.

Signed-off-by: Bruno Calza <brunoangelicalza@gmail.com>
@brunocalza brunocalza marked this pull request as ready for review December 13, 2023 19:41
@brunocalza brunocalza requested a review from avichalp December 13, 2023 19:41
)

// Vault represents a vault.
type Vault string
Copy link
Member

Choose a reason for hiding this comment

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

Have we now killed the "namespace.relation" semantics?

Copy link
Contributor

@avichalp avichalp Dec 14, 2023

Choose a reason for hiding this comment

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

interesting that we still use the dot notation in the name: https://github.com/tablelandnetwork/basin-cli/pull/28/files#diff-083575b8cef39c5a3f9bb2f511938fcf314c5755c81f0073013fd70daf5243f1R57

but now it becomes a convention to have the dot notation? should it be enforced at some layer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not yet, we can't kill it yet because the provider kind of depends on this semantics to create the namespace entry in the database. We can only kill it after remodeling the database tables (I'm planning to write a doc on how to do that remodeling, shouldn't be hard)

it's being enforced here in the CLI and here in the provider

Copy link
Member

@sanderpick sanderpick left a comment

Choose a reason for hiding this comment

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

Awesome. I left one comment about vault names, but maybe that is planned for the other renaming PR that you mentioned is planned.

// BasinUploader contains logic of uploading Parquet files to Basin Provider.
type BasinUploader struct {
namespace string
relation string
privateKey *ecdsa.PrivateKey
provider BasinProviderUploader
provider BasinProvider
Copy link
Contributor

Choose a reason for hiding this comment

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

these are private fields but do we want to keep using the old names here or it doesn't matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, makes sense to change to VaultsProvider and VaultsUploader, we can iterate on that on the next PRs

@@ -737,36 +698,6 @@ func createPublication(
}
}()

columns, err := inspectTable(ctx, tx, rel)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@avichalp avichalp left a comment

Choose a reason for hiding this comment

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

Looks good!

@brunocalza brunocalza merged commit 3d16294 into main Dec 14, 2023
2 checks passed
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.

3 participants