-
Notifications
You must be signed in to change notification settings - Fork 583
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
Ensure sub-query use for Job instances. #5563
Conversation
Fix find by name query ordering. Fix find by name not found query. Improve test case readability. #5484
Removed .andDo(print())
Removed .andDo(print())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @corneil . I have a few comments/change requests to take a look at please.
Also, is there a test that exhibits the bug this is fixing? If not, can we make one or is this a performance related issue?
...main/java/org/springframework/cloud/dataflow/server/repository/JdbcAggregateJobQueryDao.java
Outdated
Show resolved
Hide resolved
...main/java/org/springframework/cloud/dataflow/server/repository/JdbcAggregateJobQueryDao.java
Outdated
Show resolved
Hide resolved
...main/java/org/springframework/cloud/dataflow/server/repository/JdbcAggregateJobQueryDao.java
Outdated
Show resolved
Hide resolved
...main/java/org/springframework/cloud/dataflow/server/repository/JdbcAggregateJobQueryDao.java
Outdated
Show resolved
Hide resolved
.../main/java/org/springframework/cloud/dataflow/server/service/impl/DefaultTaskJobService.java
Show resolved
Hide resolved
...st/java/org/springframework/cloud/dataflow/server/controller/JobInstanceControllerTests.java
Show resolved
Hide resolved
...st/java/org/springframework/cloud/dataflow/server/controller/JobInstanceControllerTests.java
Outdated
Show resolved
Hide resolved
...st/java/org/springframework/cloud/dataflow/server/controller/JobInstanceControllerTests.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for all the one off requests, Next time I'll start a review.
...src/main/java/org/springframework/cloud/dataflow/server/repository/AggregateJobQueryDao.java
Show resolved
Hide resolved
4a9fbb1
to
d8faa9e
Compare
...st/java/org/springframework/cloud/dataflow/server/controller/JobInstanceControllerTests.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@corneil LGTM - thanks for the updates. There are still a couple of wildcard imports. Once these are adjusted consider this PR approved. I am approving now but please update the imports.
Fix find by name query ordering.
Fix find by name not found query.
Improve test case readability.
#5484