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

fix: MySQL Performance Issues in "/ids/Users" Endpoint #2859

Merged
merged 2 commits into from
Apr 30, 2024

Conversation

adrianhoelzl-sap
Copy link
Contributor

see issue #2856

@cf-gitbot
Copy link

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

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

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

@adrianhoelzl-sap adrianhoelzl-sap marked this pull request as ready for review April 30, 2024 14:41
@adrianhoelzl-sap adrianhoelzl-sap requested a review from a team April 30, 2024 14:41
@Tallicia
Copy link
Contributor

Running through tests and this should be ready to merge once we get word back from Daniel and David.

Copy link
Contributor

@Tallicia Tallicia left a comment

Choose a reason for hiding this comment

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

Looks good, would like to see at 1-2 follow up PRs for SQL generated test, and where clause refactor to use IN instead of many nested ORs

@@ -31,6 +31,7 @@
<property name="deactivateOnDelete" value="${scim.delete.deactivate:false}"/>
<property name="usernamePattern" value="${scim.username_pattern:[\p{L}+0-9+\-_.@'!]+}"/>
<property name="timeService" ref="timeService"/>
<property name="useCaseInsensitiveQueries" ref="useCaseInsensitiveQueries"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Like seeing this.


// build WHERE clause
final ProcessedFilter where = queryConverter.convert(filter, sortBy, ascending, zoneId);
final String whereClauseScimFilter = where.getSql();
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this is where the many parens and ORs are generated, this should be looked at to refactor to use an IN clause.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was surprised to see that the deep nesting has been there a long time, and wasn't the cause of the performance degradation. But still it would be much easier to read the SQL without all the nesting.

}

private static void addIdentityProvider(JdbcTemplate jdbcTemplate, String idzId, String originKey, boolean active) {
jdbcTemplate.update("insert into identity_provider (id,identity_zone_id,name,origin_key,type,active) values (?,?,?,?,'UNKNOWN',?)", UUID.randomUUID().toString(), idzId, originKey, originKey, active);
Copy link
Contributor

Choose a reason for hiding this comment

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

good addition of active

@Tallicia Tallicia self-assigned this Apr 30, 2024
@Tallicia Tallicia added in progress accepted Accepted the issue bug and removed unscheduled labels Apr 30, 2024
@Tallicia Tallicia merged commit 38a2048 into develop Apr 30, 2024
20 checks passed
@Tallicia Tallicia deleted the fix/mysql-performance-issues-ids-users-endpoint branch April 30, 2024 15:30
@cf-gitbot cf-gitbot removed in progress accepted Accepted the issue labels Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants