ebean-orm / ebean-migration

DB Migration runner (similar to Flyway) which can be used standalone or with Ebean (run migrations on EbeanServer start)
Apache License 2.0
9 stars 5 forks source link

Support "basePlatform" (e.g. support both db2 and db2luw) - Was, Problems with auto-detection of migration scripts #102

Closed rPraml closed 2 years ago

rPraml commented 2 years ago

We have currently small problems with DB-migration, because it does not feel "smooth".

@rbygrave The first question, what would be the expected directory layout, regarding the "platform"?

Should it be the platform or the base platform. e.g

1. Base-Platform
dbmigration
  + db2
  + h2
  + sqlserver

2. Platform
dbmigration
  + db2luw
  + db2zos
  + h2
  + sqlserver17

We use currently the base platform, because real platform will not work. (but the tests in ebean will use "platform" https://github.com/ebean-orm/ebean/tree/master/ebean-ddl-generator/src/test/resources/dbmigration/migrationtest) It is currently no big deal, but it will prohibit to use "db2luw" and "db2zos" (and also sqlserver16/17) which might have different migration scripts

It does not work, becase DbNameUtil.platform will return the wrong platform, if platform is "sqlserver17" and not "sqlserver"

So we are currently forced by this to set the base platform here.

Possible fixes:

Unfortunately, LocalMigrationResources.readResourcesForPath searches for resources in path + "/" + platform. If it does not find any, it will fall thru and search in path only.

The variable path is set normally to "dbmigration" So I could manipulate that value to "dbmigration/sqlserver17", but this "feels" also wrong.

I also noted, that path is scanned for global jdbc resources, which might be skipped then.

rbygrave commented 2 years ago

The general plan is to move away from "scanning" altogether.

So my thought is to pass both platform and base and for the migration runner to check those 2 options.

Instead of exact match, change to "startsWith"

That would mean it needs to scan for matches which I am trying to move away from. I'd instead prefer platform and base (2 locations)

for whatever reason... "db2luw-with-tablespaces" or "sqlserver17-customer1"

If that sort of thing is needed then people can programmatically control the whole thing and I think that would be fine.

is scanned for global jdbc resources

Ultimately I want to change JDBC migrations to use what I think is a more normal pattern of service loading (via ServiceLoader).

nPraml commented 2 years ago

Hello @rbygrave ,

I have also a question to this topic. What is the difference between platform and platformName in MigrationConfig?

I had to set the platformName to the base platform name (e.g. sqlserver) in order to generate the migration table in MigrationTable.getCreateTableScript().

rbygrave commented 2 years ago

difference between platform and platformName in MigrationConfig?

So 'platform' was added as part of https://github.com/ebean-orm/ebean-migration/issues/88 ... BUT ... I think that was a mistake in that it should have used / reused the existing platformName.

I think we need to merge these 2 fields into one. Let me confirm that @NSzemenyei

rbygrave commented 2 years ago

@NSzemenyei ... "merged" platformName and platform with https://github.com/ebean-orm/ebean-migration/issues/104

rbygrave commented 2 years ago

Hi @NSzemenyei @rPraml ... pushed the change to support basePlatform (as well as the existing platform).

The migration runner will first look for migrations for the basePlatform and then if not found look for migrations for platform (and then if not found look scan the entire path).

Ebean itself will need a matching change to set the basePlatform, and change the current call to setPlatform() to not use .base().

Just to state again, the ultimate goal is to "not scan" which gives us a fast execution path for the common case where there are no new db migrations to run. That is, ultimately the migration runner knows the basePlatform/platform, reads the idx_ file, check all the checksums contained there and knows if there are any migrations to run (plus a service load for jdbc migrations) - so no resource path scanning for that common case.

rbygrave commented 2 years ago

I'll release ebean-migration 12.16.0 ... and then we need ebean to change to make use of this support for basePlatform.

rbygrave commented 2 years ago

Closing with "fix" deemed to be in 12.16.0 (pending the related change in Ebean itself).