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

Asynchronous protocol support #2

Merged
merged 33 commits into from
May 9, 2022
Merged

Asynchronous protocol support #2

merged 33 commits into from
May 9, 2022

Conversation

NRHelmi
Copy link
Contributor

@NRHelmi NRHelmi commented Apr 20, 2022

This pull request adds the asynchronous protocol support to the go sdk:

  • Parses multipart response.
  • Parses arrow response .
  • Adds ExecuteAsync, ExecuteAsyncWait, GetTransaction, GetTransactions, GetTransactionResults, GetTransactionMetadata and GetTransactionProblems.

@NRHelmi NRHelmi marked this pull request as ready for review April 21, 2022 10:05
rai/client.go Outdated Show resolved Hide resolved
rai/client.go Outdated Show resolved Hide resolved
rai/client.go Outdated Show resolved Hide resolved
rai/client.go Outdated Show resolved Hide resolved
rai/client.go Outdated Show resolved Hide resolved
rai/client.go Show resolved Hide resolved
rai/client.go Outdated Show resolved Hide resolved
rai/client.go Show resolved Hide resolved
rai/client.go Show resolved Hide resolved
rai/client_test.go Outdated Show resolved Hide resolved
rai/client_test.go Outdated Show resolved Hide resolved
@Segflow
Copy link
Contributor

Segflow commented Apr 21, 2022

I did a first pass and will do more later.

Not really related to this PR, but I noticed that some network calls (like Client.Get) are un-cancellable by the user. I believe we should make those methods accept a context.Context parameter as a first argument to make that possible. wdyt @bradlo ?

Also it would be great if we get this package indexed in https://pkg.go.dev/ and have a badge that links to the docs in the readme. (See https://pkg.go.dev/about#creating-a-badge)

rai/client.go Outdated Show resolved Hide resolved
rai/client.go Outdated Show resolved Hide resolved
rai/client.go Outdated Show resolved Hide resolved
rai/client.go Outdated Show resolved Hide resolved
rai/client.go Show resolved Hide resolved
rai/client.go Outdated Show resolved Hide resolved
rai/client.go Outdated Show resolved Hide resolved
rai/client.go Outdated Show resolved Hide resolved
rai/client.go Outdated Show resolved Hide resolved
rai/client.go Outdated Show resolved Hide resolved
rai/client.go Show resolved Hide resolved
rai/client.go Outdated Show resolved Hide resolved
rai/client.go Outdated Show resolved Hide resolved
rai/client.go Outdated Show resolved Hide resolved
rai/client.go Outdated Show resolved Hide resolved
rai/client.go Outdated Show resolved Hide resolved
@NRHelmi
Copy link
Contributor Author

NRHelmi commented Apr 29, 2022

@Segflow the PR is ready for another check

Copy link
Contributor

@Segflow Segflow left a comment

Choose a reason for hiding this comment

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

LGTM,

I still don't know if we should return ArrowRelation and leaking the fact that we use Arrow.

If you think that's fine and that's how the other sdks are doing then we can merge this.

return &result, err
}

func (c *Client) GetTransactionResults(id string) ([]ArrowRelation, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the user know that we use Arrow? Shouldn't that be abstracted from the user?

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's what we are doing exactly for JS, Java and Csharp sdks, I believe there is a plan to address the query async results format so probably we won't keep this format in the future.

return result, err
}

func (c *Client) GetTransactionProblems(id string) ([]interface{}, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we can't avoid returning the []interface{} here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's the case since we expect two different types of problems ClientProblems and IntegrityConstraintViolation, I believe this is an option to achieve polymorphism in go

rai/models.go Outdated
DatabaseName string `json:"database_name,omitempty"`
ReadOnly bool `json:"read_only,omitempty"`
Query string `json:"query,omitempty"`
LastRequestedInterval string `json:"last_requested_interval,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a timestamp or a time or what?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

from what I see it's a string "last_requested_interval":"0s", not sure about the exact meaning

@NRHelmi NRHelmi merged commit c62e362 into main May 9, 2022
@NRHelmi NRHelmi deleted the hnr-async-protocol branch May 9, 2022 13: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.

None yet

3 participants