Add pagination feature #652
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| return result | ||
|
|
||
| def list_jobs_paginated( |
There was a problem hiding this comment.
What is the conceptual difference between a Job and a JobDetails/do we intend to expose both concepts? If yes, there needs to be more of a difference between the method names for the paginated and non-paginated responses. If they should be the same, we should not return collections of different types.
There was a problem hiding this comment.
Job and Job Details are two different items. Job Details are the result coming back from API, which only contains the job details and metadata. Job is a class that has workspace object and jobDetails in it, it has functionalities to use workspace APIs to get job results from workspace connected storage account and etc.
I was planning to ask this question during SDK review meeting that whether we need to expose the function that returns PagedItem since according to SDK guidelines, we need to expose this function. However, I was planning not expose the function since in the current implementation, we wrap the jobDetails in Job class, the user has to create a Job object that uses workspace object and the jobDetails in order to use the functions in Job class properly.
If you think we have to expose that function, maybe could you suggest a better name? for example, list_job_details_paginated?
There was a problem hiding this comment.
The azure sdk guidelines (for python) say that any methods that are called list_something should be paged operations (return an ItemPaged). But we have already made the mistake in a non-preview version of the library to ship something that is not an ItemPaged, and we can't (easily) go back and fix that.
The other issue (from an azure sdk guidelines perspective) we have is that a Job shouldn't be an active object, or it should have a Client suffix at the end.
Is this addition made only to adhere to sdk guidelines?
|
|
||
| return result | ||
|
|
||
| def list_jobs_paginated( | ||
| self, | ||
| name_match: Optional[str] = None, |
There was a problem hiding this comment.
These should be keyword-only arguments. It is too easy to accidentally provide a job_match value in the name_match position etc. and there is no intuitive ordering of parameters.
| ) | ||
| orderby = self._create_orderby(orderby_property, is_asc) | ||
|
|
||
| return client.list(subscription_id=self.subscription_id, resource_group_name=self.resource_group, workspace_name=self._workspace_name, filter=job_filter, orderby=orderby, top = page_size, skip = skip) |
There was a problem hiding this comment.
top and page_size are two different parameters. top should limit the total number of items to include across all pages. page_size is the maximum number of items in a page. What are the service semantics for the two parameters?
There was a problem hiding this comment.
It should be top. In our service, we want to use a variable to represent the number of items returned in each API request, e.g., the first request would be 30 items, then the second request would be 30 items + skip 30 items.
I will change page_size to top for consistency.
No description provided.