[WIP] Refactor jdbc migration and use service loader #151

rPraml · 2023-11-23T11:07:13Z

Hello Rob,

I've tried to implement #90

This PR removes the old java migration class path scanning.

With this change JdbcMigrations are fully provided with JdbcMigrationFactory. This is implemented by default as ServiceLoader.
It is possible to specify your own factory (which could just be a list with migration instances)

We will test this the next day here in our code base and I'll give feedback if it works.
It would be great, if I get some feedback from you, if this is going in the right direction.

Roland

ebean-migration/pom.xml

rPraml · 2023-11-23T11:08:03Z

ebean-migration/src/main/java/io/ebean/migration/ConfigurationAware.java

-  /**
-   * Set the configuration being used.
-   */
-  void setMigrationConfig(MigrationConfig config);


this is removed. I want the JdbcMigrations stateless

rPraml · 2023-11-23T11:09:23Z

ebean-migration/src/main/java/io/ebean/migration/JdbcMigration.java

   */
-  void migrate(Connection connection);
+  void migrate(Connection connection, MigrationConfig config);


This causes an API break.

I could avoid this by "default" methods, but then I think no one reads the changelog and the java migrations will not work anymore.

Ok. Yes I agree that people generally don't read release notes.

rPraml · 2023-11-23T11:11:08Z

ebean-migration/src/main/java/io/ebean/migration/runner/LocalMigrationResources.java

   */
  boolean readResources() {
    if (readFromIndex()) {
      // automatically enable earlyChecksumMode when using index file with pre-computed checksums
      migrationConfig.setEarlyChecksumMode(true);
-      return true;


I've changed the semantic of "readFromIndex" here.
It returns true, if there is an index file, although if it is empty - so do not fall back to class path scan. We can still have java migrations

rPraml · 2023-11-30T07:05:53Z

Hello @rbygrave, we did some tests with our staging apps and this would be the way we want to go.

It simplifies also the code a lot, as we do not have a dedicated java class for each platform in the src/main/java/dbmigration/PLATFORM packages. Although by default, this PR uses the ServiceLoader (#90) we use a custom factory (see below)

So this PR should make #90 and #149 obsolete.

We also have a good workaround for Problem 2 mentioned in #150
(We write a JdbcMigration now, that "injects" the test data, which honestly is even easier than doing this with raw-sql)

Problem 1 in #150 is more theoretical. We regenerate the idx file on each release, so we should notice, when someone has touched existing migrations. In future, we can think about, if ebean-migration should validate the real checksum in the file against the one in the index file, when a migration is executed.

So if we get this merged, we do not need a fork anymore and we are happy for now :)

FYI: We use spring. And spring performs a "Component"/Classpath scan on startup.
We added an interceptor to this which sees all @EbeanClasses. (Similar to BootupClasses in ebean) classpath scan. This list of classes is then added to ebeans DatabaseConfig.

For the migration, we wrote a custom factory, that reuses this list and provides instances for the JdbcMigrations. This way we do not need an other Avaje classpath scan. We do not use the serviceLoader

Roland

rob-bygrave

Getting close. Looks like there are a few small things to look at here though.

"Factory" is more a "collection of JdbcMigration"
Should there be 1 more level of indirection in terms of service loading many JdbcMigration (I think so)
Should there be an explicit getVersion() method on JdbcMigration
Should there be a JdbcMigrationContext interface rather than connection + migrationConfig

rob-bygrave · 2023-11-30T09:11:35Z

ebean-migration/src/main/java/io/ebean/migration/JdbcMigration.java

   */
-  void migrate(Connection connection);
+  void migrate(Connection connection, MigrationConfig config);


Ok. Yes I agree that people generally don't read release notes.

rob-bygrave · 2023-11-30T09:18:35Z

ebean-migration/src/main/java/io/ebean/migration/runner/LocalMigrationResources.java

+      while (iterator.hasNext()) {
+        JdbcMigration jdbcMigration = iterator.next();
+        if (jdbcMigration.matches(migrationConfig)) {
+          final var version = MigrationVersion.parse(jdbcMigration.getName());


Not sure that getName() should be used for version. I'm thinking there should be an explicit getVersion() method on the JdbcMigration interface (that implementations must provide a version).

ebean-migration/src/main/java/io/ebean/migration/runner/LocalMigrationResources.java

ebean-migration/src/main/java/io/ebean/migration/JdbcMigration.java

rob-bygrave · 2023-11-30T09:27:08Z

ebean-migration/src/main/java/io/ebean/migration/MigrationConfig.java

-        throw new IllegalArgumentException(className + " is not a valid JdbcMigration", e);
-      }
+    public Iterator<JdbcMigration> iterator() {
+      return ServiceLoader.load(JdbcMigration.class, getClassLoader()).iterator();


I'm not sure about service loading JdbcMigration (each migration). I have in my mind that it would be better to add 1 level of indirection (a JdbcMigrationSupplier or JdbcMigrationFactory) ... then typically only that 1 supplier/factory would need to be registered for service loading rather than each JdbcMigration.

Hmmm.

You have the first level of indirection in the migrationConfig. You can set your jdbcMigrationFactory/Collection by a property. So you can implement your own search strategy (we did this with our spring-classpath scan)

I think, just Service-loading the factory is not a good idea, if you have multiple ebean-servers to different databases with different schemas or even different platforms. So the jdbcMigrations have to be configured the same way as the jdbcMigration-directory

rob-bygrave · 2023-11-30T09:30:56Z

ebean-migration/src/test/java/dbmig/V1_2_1__test.java


  @Override
-  public void migrate(Connection connection) {
+  public void migrate(Connection connection, MigrationConfig config) {


In a more ideal API some interface would be here. It would be read-only (only getters/accessors).

e.g. JdbcMigrationContext with methods like:

Connection connection()

Transaction transaction() // return the connection wrapped as a ebean transaction

Platform platform() // ??

What is in MigrationConfig that JdbcMigration implementations need to access?

Good idea. We need connection and platform
Don't know how you mean that with transaction. We have built a mechanism, that we have a "working" ebean-server:

private static final ThreadLocal<Database> DB_Threadlocal = new ThreadLocal(); public void migrate(final Connection connection, MigrationConfig config) { SpiEbeanServer defaultServer = (SpiEbeanServer)DB_Threadlocal .get(); assert defaultServer != null; TransactionManager transactionManager = (TransactionManager)defaultServer.transactionManager(); SpiTransaction txn = transactionManager.wrapExternalConnection(connection); transactionManager.externalBeginTransaction(txn, TxScope.notSupported()); try { this.migrate(defaultServer); txn.flush(); } finally { transactionManager.externalRemoveTransaction(); } }

Let's focus on connection and platform at the first step. Maybe I'll extend the context in the next PR

rob-bygrave · 2023-11-30T09:33:52Z

ebean-migration/src/main/java/io/ebean/migration/MigrationConfig.java

@@ -418,17 +423,34 @@ public void setClassLoader(ClassLoader classLoader) {
  /**
   * Return the jdbcMigrationFactory.
   */
-  public JdbcMigrationFactory getJdbcMigrationFactory() {
+  public Iterable<JdbcMigration> getJdbcMigrationFactory() {


We have a name mismatch in that getJdbcMigrationFactory() returns a "collection of migrations".

So I get it but it seems off. Migrations need to be sorted into version order etc. This doesn't quite look right yet.

agree, I'll rename it to getJdbcMigrationCollection

Migrations need to be sorted into version order etc. This doesn't quite look right yet.

I don't get you here. They are sorted later here https://github.com/ebean-orm/ebean-migration/pull/151/files#diff-10ad52c1950c7a3f96d7f7286cee908ff73e79324b8414c53ce3ad17204ca9e1R57 and they have to be sorted between the SQL migrations

rPraml · 2023-11-30T12:26:26Z

@rob-bygrave thanks for feedback.

I'll update the PR the next days...

rename JdbcMigrationFactory to JdbcMigrationCollection
introduce JdbcMigrationContext
introduce explicit getVersion method

then we can discuss the left open points.

Roland

rob-bygrave · 2023-11-30T19:35:57Z

Yeah great. This is awesome by the way, thanks for taking this on!!

rob-bygrave · 2023-12-11T19:08:09Z

I hope to look at this over the next day.

rPraml

Hello @rbygrave I found some time to continue the work here.

This is not yet ready to merge, but I have one question, that we should clarify.

Do you think, it is a good idea to use the plugin-api to inject the migration run process in ebean?
Or should I better rely on the AutoMigrationRunner service, which means, that I have to change the void run(DataSource dataSource) to effectively void run(Database ebeanServer) which means, that we will get a dependency to ebean here (and ebean has a dependency to AutoMigrationRunner)

ebean-migration/src/main/java/io/ebean/migration/JdbcMigration.java

rPraml · 2024-04-11T14:09:36Z

ebean-migration-db/src/main/java/io/ebean/migration/db/MigrationContextDb.java

+public interface MigrationContextDb extends MigrationContext {
+  public Transaction transaction();
+
+  public Database database();


CHECKME: Should I add "Transaction" and "Database" in MigrationContext (means that ebean-migration will get a dependency to ebean-api)

rPraml · 2024-04-11T14:20:40Z

ebean-migration-db/src/main/java/io/ebean/migration/db/MigrationPlugin.java

+  @Override
+  public void configure(SpiServer server) {
+    config.setName(server.name());
+    config.load(server.config().getProperties());


CHECKME:

Ebean transfers some values form the DatabaseConfig

final String dbSchema = config.getDbSchema(); migrationRunner.setDefaultDbSchema(dbSchema); // mig: schema, setCurrentSchema Platform platform = config.getDatabasePlatform().platform(); migrationRunner.setBasePlatform(platform.base().name().toLowerCase()); // mig: basePlatform migrationRunner.setPlatform(platform.name().toLowerCase()); // mig: platform or platformName

Would mean that some properties have to be migrated

rPraml · 2024-04-11T14:21:58Z

ebean-migration-db/src/main/java/io/ebean/migration/db/MigrationPlugin.java

+  @Override
+  public void online(boolean online) {
+    if (online && config.isPluginRun()) {
+      new MigrationRunnerDb(config, server).run();


CHECKME: online might be too early. If we decide to implement this as plugin we might need some lifecycles in the Plugin api (e.g. a postOnline method)

rPraml · 2024-10-08T15:09:50Z

Hello @rbygrave
this is still open. I may find some time to continue with the work here.

The goal is, to avoid classpath-scanning and provide a mechnanism, that an application can define, how migrations are discovered.

This can be done by

either specify each migration in META-INF/services
specify a migration-factory in config, that does it's own discovery
specify migrations directly in config

An other goal is, that the EbeanServer is accessible during JDBC-migrations. The current API provides only a connection.
We need to do some DTO-Queries and maybe some conversions of UUIDs in the db-specific byte format during migrations.
Currently, we use a ThreadLocal, so that we can "pass" the current server to the migration

To make some progress here, I would clarify, to which API I should rely in the future.
Actually, there are two APIs, where the ebean-migration can attach to ebean

AutoMigrationRunner, which provides a connection (and no server)
Plugin-API which provides an ebeanserver (but maybe confusing)

Can you tell me, what are your preferences?

run the migration during "online" of the plugin-api (and maybe remove the AutoMigrationRunner API in future)
or extend the AutoMigrationRunner somehow, so that we can pass the ebeanserver and not only the connection? (I would need some help to sync ebean-migration-auto with ebean, as the two are a bit cross dependant)

Side-note: We need to run migrations also during runtime, not only on startup. We use ebean in the MULTI_TENANT_DB_WITH_MASTER mode. So we have one table in our master DB, where all tenants are registered. So when we detect, that a new tenant was entered, we start a new connection pool with the credentials and run the migration for this tenant.

rPraml added 4 commits November 23, 2023 08:01

removed old migration code

13f69b0

JdbcMigrations are provided by a factory now

4180c7b

Added tests and fixes for serviceLoader

4c0e7cf

Fix return value of readResources

33a356c

rPraml commented Nov 23, 2023

View reviewed changes

rob-bygrave reviewed Nov 30, 2023

View reviewed changes

Changed to collection and added context

cb73cd9

rPraml changed the title ~~Refactor jdbc migration and use service loader~~ [WIP] Refactor jdbc migration and use service loader Dec 8, 2023

Tests will run

58d893f

rPraml mentioned this pull request Dec 11, 2023

Introduce MigrationContext to be prepared for JDBC migration refactor #152

Merged

rPraml added 15 commits January 2, 2024 08:09

formatting

452a16e

Merge branch 'master-rob' into refactor-jdbc-migration

ee3eeb6

fixed compile errors

f51f437

Provide access to Database

fbf4e5f

add it-tests

a405ce0

Merge branch 'fix-shutdown-ds' into refactor-jdbc-migration

bc51a45

add it test

a66a434

Reverting some code

16aee17

begin with db dependency

e60e475

added plugin

4da3958

Merge branch 'master-rob' into refactor-jdbc-migration

2ab8ef2

FIX: Snapshot versions

40d6a73

Comments

3de01dc

Removed obsolete IT project (tests are in ebean-migration-db)

6d14cf8

Some comments and documentation

4391093

rPraml commented Apr 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Refactor jdbc migration and use service loader #151

[WIP] Refactor jdbc migration and use service loader #151

rPraml commented Nov 23, 2023

rPraml Nov 23, 2023

rPraml Nov 23, 2023

rob-bygrave Nov 30, 2023

rPraml Nov 23, 2023

rPraml commented Nov 30, 2023

rob-bygrave left a comment

rob-bygrave Nov 30, 2023

rob-bygrave Nov 30, 2023

rob-bygrave Nov 30, 2023

rPraml Nov 30, 2023

rob-bygrave Nov 30, 2023

rPraml Nov 30, 2023

rob-bygrave Nov 30, 2023

rPraml Nov 30, 2023

rPraml commented Nov 30, 2023

rob-bygrave commented Nov 30, 2023

rob-bygrave commented Dec 11, 2023

rPraml left a comment

rPraml Apr 11, 2024

rPraml Apr 11, 2024

rPraml Apr 11, 2024

rPraml commented Oct 8, 2024

[WIP] Refactor jdbc migration and use service loader #151

Are you sure you want to change the base?

[WIP] Refactor jdbc migration and use service loader #151

Conversation

rPraml commented Nov 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rPraml commented Nov 30, 2023

rob-bygrave left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rPraml commented Nov 30, 2023

rob-bygrave commented Nov 30, 2023

rob-bygrave commented Dec 11, 2023

rPraml left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rPraml commented Oct 8, 2024