Return resource count from ListXXX calls #595

yebrahim · 2018-12-26T22:22:05Z

Fixes #103.
Adds a second SQL query in the server list objects paths, which runs in a transaction to count matching objects, accounting for filter predicates, but not pagination, and adds a total_size field in list response objects to return the number. Unit and integration tests were updated to make sure the right numbers are returned.

/area back-end

This change is

IronPan · 2019-01-01T23:02:34Z

fix #103

IronPan

can you add integration test, or updating the existing integration test for this?

backend/api/experiment.proto

backend/src/apiserver/storage/experiment_store.go

backend/src/apiserver/storage/experiment_store_test.go

backend/src/apiserver/storage/job_store.go

backend/src/apiserver/storage/pipeline_store.go

IronPan · 2019-01-01T23:23:21Z

/assign @IronPan

backend/src/apiserver/storage/experiment_store.go

backend/src/apiserver/storage/job_store.go

backend/src/apiserver/storage/pipeline_store.go

backend/src/apiserver/storage/experiment_store.go

yebrahim · 2019-01-08T01:55:13Z

comments should be resolved now, PTAL.

vicaire

Thanks for addressing the comments. I found a couple more things to address.

backend/src/apiserver/storage/experiment_store.go

backend/src/apiserver/storage/job_store.go

backend/src/apiserver/storage/pipeline_store.go

backend/src/apiserver/storage/run_store.go

backend/src/apiserver/storage/pipeline_store.go

backend/src/apiserver/storage/job_store.go

yebrahim · 2019-01-09T20:18:55Z

/test kubeflow-pipeline-build-image

backend/src/apiserver/storage/job_store.go

backend/src/apiserver/storage/pipeline_store.go

backend/src/apiserver/storage/run_store.go

IronPan · 2019-01-28T09:00:03Z

backend/src/apiserver/storage/list_util.go

-	}
-	if len(results) < newContext.PageSize {
-		return results, "", nil
+func FilterOnResourceReference(tableName string, resourceType common.ResourceType, selectCount bool,


how about passing "*" and "count(*)" string instead of selectCount boolean flag. maybe consider define them as const? it would be more readable from method caller perspective.

I can do that, but selectCount is passed through from the different implementations of buildSelectXXXQuery. I think that's a better interface there, since it's being relied on to change the way the query is built. If you really want this change, do you think it should be propagated to all 4 implementations?

backend/src/apiserver/storage/list_util_test.go

IronPan · 2019-01-28T09:04:25Z

backend/src/apiserver/storage/pipeline_store.go

+		return sqlBuilder.From("pipelines").Where(sq.Eq{"Status": model.PipelineReady})
+	}
+
+	sqlBuilder := buildQuery(sq.Select("*"))


please consider define "*" and "count(*)" as const and use them here

I don't think this adds any safety, but it risks making the code less readable IMO.
What would you call these variables? Also, do you think we should make the rename everywhere?

backend/src/apiserver/storage/run_store.go

backend/src/apiserver/storage/experiment_store.go

backend/src/apiserver/storage/list_util.go

IronPan · 2019-01-28T09:21:56Z

please add integration tests for covering corresponding new API features.

yebrahim · 2019-01-30T19:04:16Z

@IronPan changed integration tests to check TotalSize as well, PTAL.

vicaire · 2019-01-30T21:43:53Z

/lgtm

IronPan · 2019-01-31T18:26:34Z

/lgtm

IronPan · 2019-01-31T18:26:40Z

/approve

k8s-ci-robot · 2019-01-31T18:26:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: IronPan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~backend/OWNERS~~ [IronPan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2019-01-31T18:26:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: IronPan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~backend/OWNERS~~ [IronPan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* See kubeflow#595 * This PR is creating a reconciler for the auto deployed clusters. * The reconciler compares the list of auto-deployed clusters against the master and release branches to determine if we need a cluster based off a newer commit. * If we do we fire off a K8s job to deploy Kubeflow. * This PR includes some general utilities * assertions.py some useful utilities for writing test assertions to compare lists and dictionaries * gcp_util some common GCP functionality like an iterator to list Deployments. * git_repo_manager.py a wrapper class to manage a local clone of a git repo.

* Create a new Kubeflow cluster to run auto-deploy v2. * kubeflow#595 we are creating a new version of the infrastructure to auto-deploy Kubeflow from master and release branches * This PR contains the GCP and Kustomize manifests for setting up a new Kubeflow cluster where this app will run. * We can't use the existing kubeflow-testing cluster because that doesn't have ISTIO or workload identity which we want for the auto-deploy app. * Remove OWNERs files

* Create skaffold configs and kustomize configs for deploying the auto_deployer. * Create a Dockerfile which just copies the source to try to allow fast rebuilds * Delete files for the old auto-deploy jobs. * Modify create_unique_kf_instance to allow setting labels that can be used to identify the auto-deployment. Miscellaneuous * Update the playbook about quota errors; unrelated to this PR but useful. Fix kubeflow#595 - AutoDeply V2

* Add a README explaining how auto deploy works. * Delete the old cron jobs since these are no longer used. Related to kubeflow#595

Resolves kubeflow#595 Signed-off-by: Christian Kadner <ckadner@us.ibm.com>

k8s-ci-robot requested review from IronPan and vicaire December 26, 2018 22:22

k8s-ci-robot added size/XL area/backend labels Dec 26, 2018

IronPan mentioned this pull request Jan 1, 2019

Return the total number of resources in list APIs #103

Closed

IronPan reviewed Jan 1, 2019

View reviewed changes

k8s-ci-robot assigned IronPan Jan 1, 2019

yebrahim force-pushed the resource-count branch from b3e5545 to 0858e22 Compare January 7, 2019 22:24

k8s-ci-robot added size/L and removed size/XL labels Jan 7, 2019

vicaire suggested changes Jan 8, 2019

View reviewed changes

k8s-ci-robot added size/XL size/XXL and removed size/L size/XL labels Jan 8, 2019

yebrahim force-pushed the resource-count branch 2 times, most recently from dbb7438 to f5b3d7d Compare January 9, 2019 00:13

vicaire suggested changes Jan 9, 2019

View reviewed changes

Yasser Elsayed added 2 commits January 24, 2019 14:16

add count to protos and libs

6298367

close db rows before second query

9ac96c5

yebrahim force-pushed the resource-count branch from d8880f4 to 48ecfad Compare January 26, 2019 01:06

IronPan reviewed Jan 28, 2019

View reviewed changes

pr comments

33c8fab

k8s-ci-robot removed the lgtm label Jan 28, 2019

yebrahim force-pushed the resource-count branch 3 times, most recently from 24c9408 to d4d996c Compare January 29, 2019 07:25

list_utils -> list

a96fc14

yebrahim force-pushed the resource-count branch from d4d996c to a96fc14 Compare January 29, 2019 08:14

Yasser Elsayed added 2 commits January 29, 2019 23:29

fix clients and fake clients to support TotalSize

f5653c0

added TotalSize checks in api integration tests

55bacd1

yebrahim force-pushed the resource-count branch from 1ca6e1d to 55bacd1 Compare January 30, 2019 18:12

k8s-ci-robot added the lgtm label Jan 30, 2019

k8s-ci-robot added the approved label Jan 31, 2019

k8s-ci-robot merged commit ec4d7e8 into kubeflow:master Jan 31, 2019

rileyjbauer mentioned this pull request Jan 31, 2019

Regenerate frontend API files now that listCount APIs are merged #757

Merged

yebrahim deleted the resource-count branch March 6, 2019 19:06

Linchin pushed a commit to Linchin/pipelines that referenced this pull request Apr 11, 2023

Add README for auto-deploy. (kubeflow#620)

6c64e6c

* Add a README explaining how auto deploy works. * Delete the old cron jobs since these are no longer used. Related to kubeflow#595

magdalenakuhn17 pushed a commit to magdalenakuhn17/pipelines that referenced this pull request Oct 22, 2023

Container port specified with logger will not work (kubeflow#595)

e1d1ba5

HumairAK pushed a commit to red-hat-data-services/data-science-pipelines that referenced this pull request Mar 11, 2024

Upgrade Python SDK to kfp==1.6.3 (kubeflow#617)

5a3ef4d

Resolves kubeflow#595 Signed-off-by: Christian Kadner <ckadner@us.ibm.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return resource count from ListXXX calls #595

Return resource count from ListXXX calls #595

yebrahim commented Dec 26, 2018 •

edited

Loading

IronPan commented Jan 1, 2019

IronPan left a comment

IronPan commented Jan 1, 2019

yebrahim commented Jan 8, 2019

vicaire left a comment

yebrahim commented Jan 9, 2019

IronPan Jan 28, 2019 •

edited

Loading

yebrahim Jan 28, 2019

IronPan Jan 28, 2019 •

edited

Loading

yebrahim Jan 28, 2019

IronPan commented Jan 28, 2019

yebrahim commented Jan 30, 2019

vicaire commented Jan 30, 2019

IronPan commented Jan 31, 2019

IronPan commented Jan 31, 2019

k8s-ci-robot commented Jan 31, 2019

k8s-ci-robot commented Jan 31, 2019

Return resource count from ListXXX calls #595

Return resource count from ListXXX calls #595

Conversation

yebrahim commented Dec 26, 2018 • edited Loading

IronPan commented Jan 1, 2019

IronPan left a comment

Choose a reason for hiding this comment

IronPan commented Jan 1, 2019

yebrahim commented Jan 8, 2019

vicaire left a comment

Choose a reason for hiding this comment

yebrahim commented Jan 9, 2019

IronPan Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

yebrahim Jan 28, 2019

Choose a reason for hiding this comment

IronPan Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

yebrahim Jan 28, 2019

Choose a reason for hiding this comment

IronPan commented Jan 28, 2019

yebrahim commented Jan 30, 2019

vicaire commented Jan 30, 2019

IronPan commented Jan 31, 2019

IronPan commented Jan 31, 2019

k8s-ci-robot commented Jan 31, 2019

k8s-ci-robot commented Jan 31, 2019

yebrahim commented Dec 26, 2018 •

edited

Loading

IronPan Jan 28, 2019 •

edited

Loading

IronPan Jan 28, 2019 •

edited

Loading