Skip to content

Improve performance of JdbcStepExecutionDao::getLastStepExecution #4798

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

quaff
Copy link
Contributor

@quaff quaff commented Mar 27, 2025

  1. Use SQL order by clause instead of Java Comparator
  2. Limit result set size to 1

@quaff
Copy link
Contributor Author

quaff commented Mar 27, 2025

Alternatively, we could use SE.STEP_EXECUTION_ID IN (SELECT MAX(STEP_EXECUTION_ID) FROM ...) to align with other queries, but there is a risk to break existing behavior since CREATE_TIME is not considered for comparing.

https://github.com/spring-projects/spring-batch/compare/main...quaff:spring-batch:patch-16?expand=1

@@ -100,6 +101,7 @@ public class JdbcStepExecutionDao extends AbstractJdbcBatchMetadataDao implement
FROM %PREFIX%JOB_EXECUTION JE
JOIN %PREFIX%STEP_EXECUTION SE ON SE.JOB_EXECUTION_ID = JE.JOB_EXECUTION_ID
WHERE JE.JOB_INSTANCE_ID = ? AND SE.STEP_NAME = ?
ORDER BY SE.CREATE_TIME DESC, SE.JOB_EXECUTION_ID DESC
Copy link
Contributor

Choose a reason for hiding this comment

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

This is ordering by job execution id while the java comparator was based on the step execution id: .thenComparing(StepExecution::getId, Comparator.reverseOrder());. Is that intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, fixed now.

@fmbenhassine fmbenhassine added the status: waiting-for-reporter Issues for which we are waiting for feedback from the reporter label Jun 3, 2025
1. Use SQL order by clause instead of Java Comparator
2. Limit result set size to 1

Signed-off-by: Yanming Zhou <[email protected]>
@fmbenhassine fmbenhassine added status: feedback-provided Issues for which the feedback requested from the reporter was provided and removed status: waiting-for-reporter Issues for which we are waiting for feedback from the reporter labels Jun 4, 2025
@hpoettker
Copy link
Contributor

hpoettker commented Jun 4, 2025

The suggested change is basically a revert of the fix for #4657.

I'm not sure whether the performance gain is worth it, compared to the risk of re-introducing the previous issue.

@quaff
Copy link
Contributor Author

quaff commented Jun 5, 2025

The suggested change is basically a revert of the fix for #4657.

I'm not sure whether the performance gain is worth it, compared to the risk of re-introducing the previous issue.

This commit will not query all matched rows but only first row.

statement.setMaxRows(1); // limit max rows to 1

if (rs.next()) {
// only first row is used.
}

@quaff
Copy link
Contributor Author

quaff commented Jun 5, 2025

Our DBA has not been able to find a solution to improve the performance with an additional index.
What we did find out is that, by removing the ORDER BY, we get the result (typically just a single row in the normal case) in a matter of milliseconds. But with the ORDER BY, it takes ~ 60 seconds.

It's weird, but I'm OK if this PR is rejected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: feedback-provided Issues for which the feedback requested from the reporter was provided
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants