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

Add postgres support for JDBIHistory #1844

Merged
merged 5 commits into from Sep 26, 2018
Merged

Conversation

@mikebell90
Copy link
Contributor

@mikebell90 mikebell90 commented Aug 31, 2018

  • adds migrations (single changeset) for PG …
  • refactors HistoryJDBI into an interface, with an abstract
    base class for shared code (limit offset using standard SQL idiom)
    and concrete subclasses for Postgres and MySQL.
  • SingularityMappers can choose to use either f_user or user column - this is needed because Postgres considers user a reserved word.
  • Modify SingularityHistoryModule to handle the new bindings
  • JDBIHistoryManager - add debug logging at TRACE level.
mikebell90 added 2 commits Aug 31, 2018
* refactors HistoryJDBI into an interface, with an abstract
base class for shared code (limit offset using standard SQL idiom)
and concrete subclasses for Postgres and MySQL.
* SingularityMappers can choose to use either f_user or user column - this is needed because Postgres considers user a reserved word.
* Modify SingularityHistoryModule to handle the new bindings
* JDBIHistoryManager - add debug logging at TRACE level.
…For now, this simply triggers the mysqljdbi
@mikebell90
Copy link
Contributor Author

@mikebell90 mikebell90 commented Aug 31, 2018

@ssalinas I deliberately split everything so.no migration or compatibility concerns would arise with mysql. They were 99.9999% compatible other than the user column and the offset limit syntax. But I thought this was cleaner in the end

sqlBuilder.append(", requestId ");
sqlBuilder.append(orderDirection.or(OrderDirection.DESC).name());
}

This comment has been minimized.

@mikebell90

mikebell90 Sep 1, 2018
Author Contributor

Only change in common code - flagging so you notice

@@ -1,19 +1,7 @@
package com.hubspot.singularity.data.history;

This comment has been minimized.

@mikebell90

mikebell90 Sep 1, 2018
Author Contributor

This class now just an interface

return history.getTaskIdHistory(requestId, deployId, runId, host, lastTaskStatus, startedBefore, startedAfter, updatedBefore, updatedAfter, orderDirection, limitStart, limitCount);

List<SingularityTaskIdHistory> taskIdHistoryList = history.getTaskIdHistory(requestId, deployId, runId, host, lastTaskStatus, startedBefore, startedAfter, updatedBefore, updatedAfter, orderDirection, limitStart, limitCount);
if (LOG.isTraceEnabled()) {

This comment has been minimized.

@mikebell90

mikebell90 Sep 1, 2018
Author Contributor

I added logging at trace level for the sql stuff. Seems a useful thing

@UseStringTemplate3StatementLocator
public abstract class MySQLHistoryJDBI extends AbstractHistoryJDBI {

@SqlUpdate("INSERT INTO requestHistory (requestId, request, createdAt, requestState, user, message) VALUES (:requestId, :request, :createdAt, :requestState, :user, :message)")

This comment has been minimized.

@mikebell90

mikebell90 Sep 1, 2018
Author Contributor

Pretty much the old jdbi history class minus the common code

…pecific postgres information as needed)

* Cleaned up logging, helper methods
@ssalinas
Copy link
Member

@ssalinas ssalinas commented Sep 7, 2018

Hey @mikebell90 thanks for the PR! Was a busy week here at HubSpot, I will try and take a closer look at this early next week 👍

@mikebell90
Copy link
Contributor Author

@mikebell90 mikebell90 commented Sep 7, 2018

Cool! Thanks

@ssalinas ssalinas added the hs_staging label Sep 10, 2018
@ssalinas
Copy link
Member

@ssalinas ssalinas commented Sep 10, 2018

PR'ed a change into your branch @mikebell90 (mikebell90#3) . Will want to double check if there is the same issue for postgres in the PostgresHistoryJDBI class. So far everything else seems to be working fine in our staging environment

@mikebell90
Copy link
Contributor Author

@mikebell90 mikebell90 commented Sep 10, 2018

@ssalinas Yeah that was dumb of me. I went on memory of MySQL that it accepted both in that order ;)

Swap limit/offset
@ssalinas ssalinas added the hs_qa label Sep 17, 2018
@ssalinas
Copy link
Member

@ssalinas ssalinas commented Sep 24, 2018

This has been pretty stable in our setup so far. I'd say let's 🚢 . We'll get this in our production branch and merged this week. Can try to cut a release hopefully this week as well

@mikebell90
Copy link
Contributor Author

@mikebell90 mikebell90 commented Sep 24, 2018

Very cool!

@ssalinas ssalinas added the hs_stable label Sep 26, 2018
@ssalinas ssalinas merged commit df0ddfd into HubSpot:master Sep 26, 2018
@ssalinas ssalinas added this to the 0.21.0 milestone Sep 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.