databricks-sdk-go
databricks-sdk-go copied to clipboard
databricks
Improve semantics for listing methods
This PR moves listing requests to an ListXXXInternal entity, which keeps all the low-level pagination request fields, like offset, limit, token, page. Original entity keeps all of the fields, except pagination-related. This allows to mitigate a common confusion, where users supply limit for the iterator and it acting only a result page limit, not the max number of results.
Follow-up Python SDK PR: https://github.com/databricks/databricks-sdk-py/pull/279
Is the idea that the limit field in the request is only used for the page size and not the total number of responses? I thought you would just be able to remove https://github.com/databricks/databricks-sdk-go/blob/main/.codegen/api.go.tmpl#L219-L221, then no global limit is enforced and this API behaves the same as in the other SDKs.
Rather than supporting a global limit, we should instead switch to the lazy iterator approach and allow users to pull as many resources as they need.
@mgyucht https://github.com/databricks/databricks-sdk-go/blob/main/.codegen/api.go.tmpl#L219-L221 i think we need to change this to max_results "virtual field", that would do a "global limit". Or we can keep the limit name, but redefine the semantics for when they're used in iterators. It's necessary for things like CLI. e.g. get the latest XX jobs or show the latest XX queries.
Once I get around to https://github.com/databricks/databricks-sdk-go/pull/543, users can define their own iterators and filter however they like. IMO I don't think it needs to be built into the Go SDK, and it isn't in any other SDK we publish today.
@mgyucht it's orthogonal to #543 (we should hold on that because of the upcoming language changes - https://github.com/golang/go/issues/61405).
this change is about cross-SDK semantics on removing low-level fields to paginated methods, so it should be merged ;)
Codecov Report
Patch coverage: 1.55% and project coverage change: +0.07% :tada:
Comparison is base (
a3c5a3c) 17.97% compared to head (b55e389) 18.04%. Report is 2 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #569 +/- ##
==========================================
+ Coverage 17.97% 18.04% +0.07%
==========================================
Files 85 85
Lines 9805 9990 +185
==========================================
+ Hits 1762 1803 +41
- Misses 7889 8030 +141
- Partials 154 157 +3
| Files Changed | Coverage Δ | |
|---|---|---|
| openapi/code/method.go | 35.36% <ø> (+17.62%) |
:arrow_up: |
| service/catalog/api.go | 0.00% <0.00%> (ø) |
|
| service/catalog/impl.go | 0.00% <ø> (ø) |
|
| service/catalog/model.go | 0.00% <ø> (ø) |
|
| service/compute/api.go | 12.46% <0.00%> (-0.20%) |
:arrow_down: |
| service/compute/impl.go | 10.74% <ø> (ø) |
|
| service/compute/model.go | 0.00% <ø> (ø) |
|
| service/iam/model.go | 0.00% <ø> (ø) |
|
| service/jobs/api.go | 0.00% <0.00%> (ø) |
|
| service/jobs/impl.go | 0.00% <ø> (ø) |
|
| ... and 14 more |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Can we prepare the changes in the other SDKs at the same time, since we're making changes to the template logic? This change seems good to me, but to minimize clashes with others developing on the SDKs, I want to decrease the wall-clock time that the OpenAPI generator won't work with the other SDKs.