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.

Sign up for GitHub

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

Jump to bottom

ROX-18155: pg generic store: DeleteByQuery #6774

Merged
merged 1 commit into from Jul 7, 2023
Merged

ROX-18155: pg generic store: DeleteByQuery #6774

merged 1 commit into from Jul 7, 2023

Conversation

janisz
Copy link
Contributor

@janisz janisz commented Jun 30, 2023

Description

A detailed explanation of the changes in your PR.

Feel free to remove this section if it is overkill for your PR, and the title of your PR is sufficiently descriptive.

Checklist

  • Investigated and inspected CI test results
  • Unit test and regression tests added
  • Evaluated and added CHANGELOG entry if required
  • Determined and documented upgrade steps
  • Documented user facing changes (create PR based on openshift/openshift-docs and merge into rhacs-docs)

If any of these don't apply, please comment below.

Testing Performed

TODO(replace-me)
Use this space to explain how you tested your PR, or, if you didn't test it, why
you did not do so. Valid reasons include, for example, "CI is sufficient",
"No testable changes". Feel free to attach JSON snippets, curl commands,
screenshots.

In addition to reviewing your code, reviewers must also review your testing
instructions and make sure they are sufficient.

@janisz janisz requested review from a team as code owners June 30, 2023 16:21
This was referenced Jun 30, 2023
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
@janisz
Copy link
Contributor Author

janisz commented Jun 30, 2023 •
edited

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

This was referenced Jun 30, 2023
Merged
Merged
Merged
Merged
@roxbot
Copy link
Contributor

roxbot commented Jun 30, 2023 •
edited

Images are ready for the commit at fb141f1.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.1.x-342-gfb141f1465.

@janisz janisz mentioned this pull request Jun 30, 2023
Closed
5 tasks
@janisz janisz force-pushed the master-janisz/06-30-ROX-18155_pg_generic_store_GetMany branch from 42ab190 to 91f09f5 Compare July 3, 2023 15:47
@janisz janisz force-pushed the master-janisz/06-30-ROX-18155_pg_generic_store_DeleteByQuery branch from 45ea17e to 6766f93 Compare July 3, 2023 15:47
@janisz janisz force-pushed the master-janisz/06-30-ROX-18155_pg_generic_store_GetMany branch from 91f09f5 to 36679e0 Compare July 3, 2023 16:02
@janisz janisz force-pushed the master-janisz/06-30-ROX-18155_pg_generic_store_DeleteByQuery branch from 6766f93 to 2e100d1 Compare July 3, 2023 16:03
@janisz janisz force-pushed the master-janisz/06-30-ROX-18155_pg_generic_store_GetMany branch from 36679e0 to efaad61 Compare July 4, 2023 13:00
@janisz janisz force-pushed the master-janisz/06-30-ROX-18155_pg_generic_store_DeleteByQuery branch from 2e100d1 to ddfc8a1 Compare July 4, 2023 13:00
@janisz janisz force-pushed the master-janisz/06-30-ROX-18155_pg_generic_store_GetMany branch from efaad61 to 043cf9f Compare July 4, 2023 13:57
@janisz janisz force-pushed the master-janisz/06-30-ROX-18155_pg_generic_store_DeleteByQuery branch from ddfc8a1 to 3f6dff3 Compare July 4, 2023 13:57
rhybrillou
rhybrillou approved these changes Jul 5, 2023
View reviewed changes
pkg/search/postgres/store.go

// DeleteByQuery removes the objects from the store based on the passed query.
func (s *GenericStore[T, PT]) DeleteByQuery(ctx context.Context, query *v1.Query) error {
defer s.setPostgresOperationDurationTime(time.Now(), ops.Remove)
Copy link
Contributor

Choose a reason for hiding this comment

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

While not directly linked to the change at hand, it might be good to differentiate Delete from DeleteByQuery in the stat histograms. If the filter of DeleteByQuery is complex enough, this would introduce false outliers in the distribution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally we should track time and length of the query together so we can see correlation. Anyway I think it might be hard after this simplification: #6775 (comment)

@janisz janisz force-pushed the master-janisz/06-30-ROX-18155_pg_generic_store_GetMany branch from 043cf9f to d8a82a8 Compare July 6, 2023 11:02
@janisz janisz force-pushed the master-janisz/06-30-ROX-18155_pg_generic_store_DeleteByQuery branch from 3f6dff3 to f8c2863 Compare July 6, 2023 11:02
@janisz janisz force-pushed the master-janisz/06-30-ROX-18155_pg_generic_store_GetMany branch from d8a82a8 to a55c307 Compare July 6, 2023 14:15
@janisz janisz force-pushed the master-janisz/06-30-ROX-18155_pg_generic_store_DeleteByQuery branch from f8c2863 to 8eda922 Compare July 6, 2023 14:16
@janisz janisz force-pushed the master-janisz/06-30-ROX-18155_pg_generic_store_GetMany branch from a55c307 to 2cffae6 Compare July 6, 2023 15:36
@janisz janisz force-pushed the master-janisz/06-30-ROX-18155_pg_generic_store_DeleteByQuery branch 2 times, most recently from c9fa819 to 182c002 Compare July 6, 2023 15:47
@janisz janisz force-pushed the master-janisz/06-30-ROX-18155_pg_generic_store_GetMany branch from 2cffae6 to e17c60a Compare July 7, 2023 12:00
@janisz janisz force-pushed the master-janisz/06-30-ROX-18155_pg_generic_store_DeleteByQuery branch from 182c002 to 67da9dd Compare July 7, 2023 12:00
@janisz janisz force-pushed the master-janisz/06-30-ROX-18155_pg_generic_store_GetMany branch from e17c60a to 569f2ba Compare July 7, 2023 14:20
Base automatically changed from master-janisz/06-30-ROX-18155_pg_generic_store_GetMany to master July 7, 2023 15:32
@janisz
Copy link
Contributor Author

janisz commented Jul 7, 2023

Graphite rebased this pull request after merging its parent, because this pull request is set to merge when ready.

@janisz janisz force-pushed the master-janisz/06-30-ROX-18155_pg_generic_store_DeleteByQuery branch from 67da9dd to fb141f1 Compare July 7, 2023 15:33
@openshift-ci
Copy link

openshift-ci bot commented Jul 7, 2023

@janisz: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/ocp-4-13-core-bpf-qa-e2e-tests 182c002 link false /test ocp-4-13-core-bpf-qa-e2e-tests
ci/prow/ocp-4-10-qa-e2e-tests 67da9dd link false /test ocp-4-10-qa-e2e-tests
ci/prow/ocp-4-10-sensor-integration-tests fb141f1 link false /test ocp-4-10-sensor-integration-tests
ci/prow/ocp-4-13-operator-e2e-tests fb141f1 link false /test ocp-4-13-operator-e2e-tests

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@janisz janisz merged commit 06213bf into master Jul 7, 2023
33 of 39 checks passed
@janisz janisz deleted the master-janisz/06-30-ROX-18155_pg_generic_store_DeleteByQuery branch July 7, 2023 17:32
@janisz
Copy link
Contributor Author

janisz commented Jul 7, 2023

@janisz merged this pull request with Graphite.

This was referenced Jul 12, 2023
Merged
Merged
Merged
Merged
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhybrillou rhybrillou rhybrillou approved these changes

Assignees
No one assigned
Labels
area/central area/postgres
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@janisz @roxbot @rhybrillou