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

Filter getRequestHistory queries by createdAt times #2117

Merged
merged 4 commits into from
Jul 17, 2020

Conversation

fabatef
Copy link
Contributor

@fabatef fabatef commented Jul 15, 2020

Instead of looping through the entire request history for some requestId to find the SingularityRequestHistorys in a specific time range, this change allows us to get the request history entries filtered by createdBefore and createdAfter times via SingularityClient#getHistoryForRequest

@fabatef fabatef marked this pull request as draft July 15, 2020 23:48
@@ -253,5 +241,10 @@ void setDeployJson(
@Bind("json") @Json SingularityDeployHistory deployHistory
);

@Override
default String getRequestHistoryBaseQuery() {
Copy link
Member

Choose a reason for hiding this comment

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

If this is just a constant let's declare it as a string on the class, not as a method, same for the mysql jdbi version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The user column is named user in the mysql version, and f_user in the postgres version. That's why I didn't declare it as a constant.

Copy link
Member

Choose a reason for hiding this comment

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

ah, totally read over that difference. It can still just be a String REQUEST_HISTORY_BASE_QUERY = "..."; on each class though, vs a method

Copy link
Member

Choose a reason for hiding this comment

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

sry, read that again, ignore me...

@ssalinas
Copy link
Member

Just the one comment on the static string vs method. Only other thing is to confirm that the history paging in the UI still seems to work as intended once deployed on staging 👍 , will give the ⛵ once you've tested there

@fabatef
Copy link
Contributor Author

fabatef commented Jul 16, 2020

Tested on iad03-test via api goggles. I've spot-checked the request history paging in the UI as well, and things look good.

@fabatef fabatef added the hs_qa label Jul 16, 2020
@ssalinas
Copy link
Member

🚢

@ssalinas ssalinas marked this pull request as ready for review July 16, 2020 19:42
@fabatef fabatef changed the title WIP: Filter getRequestHistory queries by createdAt times Filter getRequestHistory queries by createdAt times Jul 16, 2020
@ssalinas ssalinas merged commit b3d309f into master Jul 17, 2020
@ssalinas ssalinas deleted the get-request-history-by-createAt-times branch July 17, 2020 12:39
@ssalinas ssalinas added this to the 1.3.0 milestone Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants