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

Optimize "/ids/Users" endpoint #2704

Merged
merged 22 commits into from
Mar 10, 2024
Merged

Optimize "/ids/Users" endpoint #2704

merged 22 commits into from
Mar 10, 2024

Conversation

adrianhoelzl-sap
Copy link
Contributor

@adrianhoelzl-sap adrianhoelzl-sap commented Feb 2, 2024 •
edited
Loading

see issue #2705

Prerequisite: PR #2702

@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Feb 2, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/186970777

The labels on this github issue will be updated when the story is started.

@adrianhoelzl-sap adrianhoelzl-sap linked an issue Feb 2, 2024 that may be closed by this pull request
Performance Issues in SCIM User Lookup of "/ids/Users" Endpoint #2705
Closed
@adrianhoelzl-sap adrianhoelzl-sap marked this pull request as ready for review February 2, 2024 13:17
@adrianhoelzl-sap adrianhoelzl-sap requested a review from a team February 2, 2024 16:10
@jarias-lfx jarias-lfx mentioned this pull request Feb 2, 2024
Closed
13 tasks
@peterhaochen47 peterhaochen47 mentioned this pull request Feb 2, 2024
Closed
@WillsonHG
Copy link

/easycla

@peterhaochen47
Copy link
Member

see issue #2705

Prerequisite: PR #2703

Hi @adrianhoelzl-sap, #2703 is an issue, did you mean PR #2702?

@adrianhoelzl-sap
Copy link
Contributor Author

see issue #2705
Prerequisite: PR #2703

Hi @adrianhoelzl-sap, #2703 is an issue, did you mean PR #2702?

You are right, I updated it. Thanks :)

@adrianhoelzl-sap adrianhoelzl-sap marked this pull request as draft February 8, 2024 17:49
@adrianhoelzl-sap adrianhoelzl-sap marked this pull request as ready for review February 13, 2024 09:28
@bruce-ricard
Copy link
Contributor

The first commit 955cd2b is already on develop . Should we rebase the PR branch?

@bruce-ricard
Copy link
Contributor

Also, github is saying

This branch has conflicts that must be resolved

bruce-ricard
bruce-ricard previously requested changes Feb 26, 2024
View reviewed changes
Copy link
Contributor

@bruce-ricard bruce-ricard left a comment

Choose a reason for hiding this comment

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

Rebase and merge conficts need to be resolved.

swalchemist
swalchemist suggested changes Mar 7, 2024
View reviewed changes
Copy link
Contributor

@swalchemist swalchemist left a comment

Choose a reason for hiding this comment

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

I'm partway through the PR.

server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java Outdated Show resolved Hide resolved
server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java Show resolved Hide resolved
swalchemist
swalchemist reviewed Mar 8, 2024
View reviewed changes
...c/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java Outdated Show resolved Hide resolved
...c/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java
when(identityZoneManager.getCurrentIdentityZoneId()).thenReturn(idzId);
}

private void arrangeScimUsersForFilter(
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is really complex. Is it really necessary to validate all of these details for all of the tests that call this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume that you were referring to the method below, i.e., assertEndpointReturnsCorrectResult. I simplified it a bit in commit d0cf81f.

...c/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java Outdated Show resolved Hide resolved
@swalchemist swalchemist dismissed bruce-ricard’s stale review March 8, 2024 22:57

No need to block on merge conflicts that have to be fixed anyway

@adrianhoelzl-sap adrianhoelzl-sap merged commit 1324a81 into develop Mar 10, 2024
20 checks passed
@adrianhoelzl-sap adrianhoelzl-sap deleted the fix/optimize-ids-users-endpoint branch March 10, 2024 19:51
@Tallicia
Copy link
Contributor

@adrianhoelzl-sap @strehle This PR is where I am very likely going to revert in the next half hour to hour as this looks to be the culprit for the performance drop.

Thoughts?

bruce-ricard added a commit that referenced this pull request Apr 29, 2024
@bruce-ricard
Fix: performance issue in MySQL
170f224 
* Revert "Merge pull request #2704 from cloudfoundry/fix/optimize-ids-users-endpoint"

* This reverts commit 1324a81, reversing
  changes made to b8826e1.

* The original PR is suspected to cause the performance issues. We
  will try to bring this change back later.
@bruce-ricard bruce-ricard mentioned this pull request Apr 29, 2024
Merged
Tallicia pushed a commit that referenced this pull request Apr 29, 2024
@bruce-ricard @Tallicia
Fix: performance issue in MySQL
2ffb7af 
* Revert "Merge pull request #2704 from cloudfoundry/fix/optimize-ids-users-endpoint"

* This reverts commit 1324a81, reversing
  changes made to b8826e1.

* The original PR is suspected to cause the performance issues. We
  will try to bring this change back later.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swalchemist swalchemist swalchemist approved these changes

@strehle strehle Awaiting requested review from strehle

@tack-sap tack-sap Awaiting requested review from tack-sap

@bruce-ricard bruce-ricard Awaiting requested review from bruce-ricard

Assignees
No one assigned
Labels
None yet
Projects
Foundational Infrastructure Working Group
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Performance Issues in SCIM User Lookup of "/ids/Users" Endpoint
7 participants
@adrianhoelzl-sap @cf-gitbot @WillsonHG @peterhaochen47 @bruce-ricard @Tallicia @swalchemist