cloudfoundry-community / autosleep

Auto sleep service for CloudFoundry
Apache License 2.0
39 stars 21 forks source link

Postgresql support for Autosleep #254

Closed pradyutsarma closed 7 years ago

pradyutsarma commented 7 years ago
  1. DB Changelog updates for PG support
  2. JPA/Hibernate strategy updates

Note: I have not provided a local profile for postgresql in this CL. Coming up in a later PR.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 95.147% when pulling 34475decbf70c0af142f20f335d10b870ac7dbe1 on pradyutsarma:develop into 08f8d663dea272799d5556c1ed83c1327e490d31 on cloudfoundry-community:develop.

gberche-orange commented 7 years ago

thanks @pradyutsarma for your PR

We'll review in details the changes and provide more detailed feedback in the coming days.

In the meantime, it might be worth considering extending the current mysql integrations tests to also run on posgresql in order to ensure tests with catch potential future regressions on the pg support. This includes the travis-ci execution that would be extended to supports pg: see https://github.com/Orange-OpenSource/autosleep/blob/develop/.travis.yml#L15 and https://docs.travis-ci.com/user/database-setup/#postgresql

pradyutsarma commented 7 years ago

Thanks @gberche-orange That's the plan. I will look into the travis-CI build and submit a separate PR to also support that. I think what we also need are tests that can also catch any issues that may occur with db changeset updates on an existing installation.

gberche-orange commented 7 years ago

This changes implies breaking the backward compatibility from the 1.0 release to the next 1.1 release, as liquibase stores checksums of applied changes on a given db. The changeset with id being modified, it will report incorrect checksum.

I could not come with a simple way to preserve backward compatibility for existing mysql databases ( even by bypassing the checksum check, or introducing the type changes into a distinct changeset).

So we're rather asking our ops people to delete all autosleep bindings and recreate it to include pg support in next version.

There are some schema enhancements we could consider in the future, including converting the ApplicationInfo.lastCheck from a BLOB type into a DATETIME.

   tableName: ApplicationInfo
   columnDataType: BLOB

/CC @arnaudruffin @antechrestos

Would it be a good time to fix this as part of this incompatible change or could we safely defer this later ?

pradyutsarma commented 7 years ago

@gberche-orange,

changes implies breaking the backward compatibility from the 1.0 release to the next 1.1 Did you check this by running against an existing DB if there really is an issue? I did do a minor test of updating an existing DB created via the initial changeset with the new one and it seemed to go through (Did not do this with data though)

If this is a real concern (which I previously raised and compulsorily needs to be managed), we can have the new changeset run only for Postgresql, but even then we run the risk of script/changeset fragmentation. However, if we agree to release 1.1 too as an incompatible release for existing deployments on autosleep (assuming updates really don't work), then do we need to maintain two changesets in the db changelog?

Actually liquibase supports a generic BOOLEAN type Thanks for pointing this out. The last time I tried this without the quotes, didn't work. I will run some tests again and update the PR.

antechrestos commented 7 years ago

@gberche-orange we could indeed break every checksum and make a single one with the valid names with underscores and the BOOLEAN type if you are ready to break your instances :smirk:

gberche-orange commented 7 years ago

@pradyutsarma

I did do a minor test of updating an existing DB created via the initial changeset with the new one and it seemed to go through (Did not do this with data though) Did you check this by running against an existing DB if there really is an issue?

Sorry, I still have to run this manual test. These days are quite busy, I'm trying to fit that in. That'd be great if your PR was indeed compatible, will double check.

However, if we agree to release 1.1 too as an incompatible release for existing deployments on autosleep (assuming updates really don't work), then do we need to maintain two changesets in the db changelog?

Yes, I agree release 1.1 can be an incompatible release for existing deployments.

@antechrestos

we could indeed break every checksum and make a single one with the valid names with underscores and the BOOLEAN type if you are ready to break your instance

The addition of a distinct changeset id=2 in https://github.com/cloudfoundry-community/autosleep/pull/254/files#diff-587bbfadf49eb633c8ae4c16b5623665R151 to rename the columns made sense to me, rather than editing the changeset id=1 directly.

In the future, we'll add new changeset for upcoming features such as space autoenrollments without changing changesets with id=1 and id=2

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 95.147% when pulling d8a9cdd19e05a7d3a10370283f26ea3297914268 on pradyutsarma:develop into 08f8d663dea272799d5556c1ed83c1327e490d31 on cloudfoundry-community:develop.

gberche-orange commented 7 years ago

@pradyutsarma

my manual test from a populated autosleep 1.0 db to a snapshot version with your PR version (commit https://github.com/pradyutsarma/autosleep/commit/34475decbf70c0af142f20f335d10b870ac7dbe1) indicates the changeset 0 checksum was not rejected on mysql with existing data (the SpaceEnrollerConfig table) and all tables names were properly converted to snake_case.

2016-09-22T14:23:49.27+0200 [APP/0]      ERR INFO 9/22/16 12:23 PM: liquibase: Successfully acquired change log lock
2016-09-22T14:23:50.60+0200 [APP/0]      ERR INFO 9/22/16 12:23 PM: liquibase: Reading from cf_9dafca67_3da3_4ca4_be3f_d05e9857c1aa.DATABASECHANGELOG
2016-09-22T14:23:50.65+0200 [APP/0]      ERR INFO 9/22/16 12:23 PM: liquibase: Successfully released change log lock

This is good news.

Sorry for the wrong assumption my side from just reading the changeset.

gberche-orange commented 7 years ago

Same result with basic manual test of https://github.com/pradyutsarma/autosleep/commit/d8a9cdd19e05a7d3a10370283f26ea3297914268

Thanks @pradyutsarma

pradyutsarma commented 7 years ago

Great @gberche-orange. That's what I thought. Now we can get on with the next set of submissions.