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

Use Jobs API 2.1 with name filter for databricks_job data source #1744

Merged
merged 2 commits into from
Nov 10, 2022

Conversation

alexott
Copy link
Contributor

@alexott alexott commented Nov 9, 2022

This patch switches to 2.1 version of Jobs API for List operation so we can more efficiently handle workspaces with huge number of jobs (> 3000). It also uses new name parameter to search jobs by name in the databricks_job data source.

This patch switches to 2.1 version of Jobs API for List operation so we can more
efficiently handle workspace with huge number of jobs (> 3000).  It also uses new `name`
parameter to search jobs by name in the `databricks_job` data source.
@alexott alexott requested review from nfx and a team November 9, 2022 17:40
}
offset += len(resp.Jobs)
}
return jobs, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

go sdk will generate the following logic. please reuse

var results []Job
	ctx = useragent.InContext(ctx, "sdk-feature", "pagination")
	for {
		response, err := a.List(ctx, request)
		if err != nil {
			return nil, err
		}
		if len(response.Jobs) == 0 {
			break
		}
		for _, v := range response.Jobs {
			results = append(results, v)
		}
		request.Offset += int(len(response.Jobs))
	}
	return results, nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we'll need to look for SDK modification as it's doing the extra API call at the end.

})
}

func TestJobsAPIListMultiplePages(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what about duplicates that appear when job is added in the middle of iteration? :) you can skip this one here, though

@nfx nfx enabled auto-merge (squash) November 10, 2022 13:16
@nfx nfx disabled auto-merge November 10, 2022 13:17
@codecov-commenter
Copy link

Codecov Report

Merging #1744 (e4d707a) into master (87e590d) will increase coverage by 0.00%.
The diff coverage is 92.85%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1744   +/-   ##
=======================================
  Coverage   89.98%   89.98%           
=======================================
  Files         141      141           
  Lines       11148    11171   +23     
=======================================
+ Hits        10031    10052   +21     
- Misses        712      713    +1     
- Partials      405      406    +1     
Impacted Files Coverage Δ
jobs/data_jobs.go 73.33% <ø> (ø)
jobs/resource_job.go 94.89% <89.47%> (-0.50%) ⬇️
exporter/importables.go 90.71% <100.00%> (ø)
exporter/util.go 83.09% <100.00%> (ø)
jobs/data_job.go 88.46% <100.00%> (+2.74%) ⬆️

@nfx nfx merged commit 510f600 into master Nov 10, 2022
@nfx nfx deleted the jobs-list-2.1-api branch November 10, 2022 13:54
@nfx nfx mentioned this pull request Nov 11, 2022
michael-berk pushed a commit to michael-berk/terraform-provider-databricks that referenced this pull request Feb 15, 2023
…atabricks#1744)

This patch switches to 2.1 version of Jobs API for List operation so we can more
efficiently handle workspace with huge number of jobs (> 3000).  It also uses new `name`
parameter to search jobs by name in the `databricks_job` data source.
@Aakash-NucleusTeq
Copy link

bn

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