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

[PECO-1015] Add support for staging operations to Go Driver #164

Merged
merged 9 commits into from
Sep 28, 2023

Conversation

nithinkdb
Copy link
Contributor

We need to add support for staging operations to the go driver. This PR enabled Get, Delete, and Put operations.

internal/config/config.go Outdated Show resolved Hide resolved
connection.go Show resolved Hide resolved
for k, v := range headers {
req.Header.Set(k, v)
}
res, err := client.Do(req)

Choose a reason for hiding this comment

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

are we doing any retries for all the client.do calls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, that seems like overkill.

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 will leave it up to Raymond, if that's a necessary feature we can implement it, but i don't believe we do these retries in go or python

Choose a reason for hiding this comment

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

Note that in Python we do retry these requests automatically. Presumably that happens automatically in Node as well because we use axios. So Jade's feedback makes sense here. Does the Go http library ever automatically retry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Outside of retriable libraries, no. https://github.com/hashicorp/go-retryablehttp -> we could hypothetically use this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the very least, this seems like follow up PR material given that it is a little more involved. This PR has been sitting a bit, and doesn't really break anything. I think we should prioritize getting it in, unless we see customers complain about retry issues.

connection.go Outdated Show resolved Hide resolved
connection.go Outdated Show resolved Hide resolved
connection.go Outdated Show resolved Hide resolved
connection.go Outdated Show resolved Hide resolved
connector.go Outdated Show resolved Hide resolved
connector.go Outdated Show resolved Hide resolved
internal/config/config.go Outdated Show resolved Hide resolved
internal/config/config.go Outdated Show resolved Hide resolved
internal/config/config.go Outdated Show resolved Hide resolved
Copy link
Contributor

@rcypher-databricks rcypher-databricks 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. Just need to fix the build and linting errors.

@susodapop
Copy link

Are these for staging operations or UC Volumes?

Copy link

@susodapop susodapop left a comment

Choose a reason for hiding this comment

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

Overall looks good but requires tests to verify the behaviour of allowed local paths.

for k, v := range headers {
req.Header.Set(k, v)
}
res, err := client.Do(req)

Choose a reason for hiding this comment

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

Note that in Python we do retry these requests automatically. Presumably that happens automatically in Node as well because we use axios. So Jade's feedback makes sense here. Does the Go http library ever automatically retry?

connection.go Show resolved Hide resolved
@nithinkdb
Copy link
Contributor Author

Are these for staging operations or UC Volumes?

This is for UC Volumes

Signed-off-by: nithinkdb <nithin.krishnamurthi@databricks.com>
Signed-off-by: nithinkdb <nithin.krishnamurthi@databricks.com>
Signed-off-by: nithinkdb <nithin.krishnamurthi@databricks.com>
Signed-off-by: nithinkdb <nithin.krishnamurthi@databricks.com>
Signed-off-by: nithinkdb <nithin.krishnamurthi@databricks.com>
Signed-off-by: nithinkdb <nithin.krishnamurthi@databricks.com>
Signed-off-by: nithinkdb <nithin.krishnamurthi@databricks.com>
Signed-off-by: nithinkdb <nithin.krishnamurthi@databricks.com>
Signed-off-by: nithinkdb <nithin.krishnamurthi@databricks.com>
@nithinkdb nithinkdb merged commit 5a3a210 into databricks:main Sep 28, 2023
3 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.

4 participants