This PR is mostly in alignment with the test PR I submitted #847 ages ago that upgraded HSQLDB (which incidentally also included thoughts and commits about Lucene upgrades). I've been using a derivative of that PR for about a year now, without issues.

Thank you, @fxthomas !

I just tested the upgrade from a fresh 10.4.1 installation and it seems to be working as expected.
Here is the relevant part of the log:

2019-10-19 22:31:33.960  INFO --- o.a.p.dao.LegacyHsqlDaoHelper            : HSQLDB database upgrade needed, from version 1.8.0 to 2.5.
2019-10-19 22:31:33.964  INFO --- o.a.p.dao.LegacyHsqlDaoHelper            : Backing up HSQLDB database to /Users/xxx/airsonic2/db/airsonic.backup.20191019223133...
2019-10-19 22:31:33.969  INFO --- o.a.p.dao.LegacyHsqlDaoHelper            : Copying /Users/xxx/airsonic2/db to /Users/xxx/airsonic2/db/airsonic.backup.20191019223133
2019-10-19 22:31:33.970  INFO --- o.a.p.dao.LegacyHsqlDaoHelper            : Copying /Users/xxx/airsonic2/db/airsonic.properties to /Users/xxx/airsonic2/db/airsonic.backup.20191019223133/airsonic.properties
2019-10-19 22:31:33.972  INFO --- o.a.p.dao.LegacyHsqlDaoHelper            : Copying /Users/xxx/airsonic2/db/airsonic.script to /Users/xxx/airsonic2/db/airsonic.backup.20191019223133/airsonic.script
2019-10-19 22:31:33.976  INFO --- o.a.p.dao.LegacyHsqlDaoHelper            : HSQLDB database backup complete.
2019-10-19 22:31:33.976  INFO --- o.a.p.dao.LegacyHsqlDaoHelper            : Performing database upgrade...
2019-10-19 22:31:34.382  INFO --- o.a.p.dao.LegacyHsqlDaoHelper            : Database connection established. Current version is: 2.5.0.
2019-10-19 22:31:34.382  INFO --- o.a.p.dao.LegacyHsqlDaoHelper            : Shutting down database...
2019-10-19 22:31:34.502  INFO --- o.a.p.dao.LegacyHsqlDaoHelper            : Database connection closed. Current version is: 2.5.0.
2019-10-19 22:31:34.502  INFO --- o.a.p.dao.LegacyHsqlDaoHelper            : Database upgrade complete. Current version is: 2.5.0.
2019-10-19 22:31:34.606  INFO --- o.a.p.service.SettingsService            : Java: 11.0.3, OS: Mac OS X
2019-10-19 22:31:34.856  INFO --- org.airsonic.player.Application          : Detected Tomcat web server
2019-10-19 22:31:39.432  INFO --- o.a.p.service.PodcastService             : Automatic Podcast update scheduled to run every 24 hour(s), starting at Sat Oct 19 22:36:39 JST 2019
2019-10-19 22:31:39.536  INFO --- o.a.p.s.search.IndexManager              : Index directory was created (index version 16). 
2019-10-19 22:31:39.537  INFO --- o.a.p.s.MediaScannerService              : Automatic media library scanning scheduled to run every 1 day(s), starting at Sun Oct 20 03:00:00 JST 2019
2019-10-19 22:31:40.490  INFO --- l.executor.jvm.JdbcExecutor              : SELECT COUNT(*) FROM PUBLIC.DATABASECHANGELOGLOCK
2019-10-19 22:31:40.492  INFO --- l.executor.jvm.JdbcExecutor              : SELECT COUNT(*) FROM PUBLIC.DATABASECHANGELOGLOCK
2019-10-19 22:31:40.500  INFO --- l.l.StandardLockService                  : Successfully acquired change log lock

Let me know if you need more info.
And feel free to close #876 .

@harawata Thanks a lot for the testing! I'll close the old PR then.
@randomnicode Ah, thanks as well for the information! I'm really glad to know that there are no issues with running HSQLDB 2.x for an extended period of time.

One thing I'm still uneasy about is that the controlled upgrade procedure requires that nothing opens a connection to the data source while we're doing it (otherwise the database would be in fact upgraded, but without backup, and without the additional protection of doing SHUTDOWN SCRIPT, which we cannot do on the main data source object).

During my tests this was the case, but with how it is currently setup this is not in fact guaranteed. I was wondering if there was a better way?

@fxthomas ,

In my understanding, airsonic uses HSQLDB in file: mode and there can be no multiple connections to the database.

If we need to care about users who use HSQLDB in server-client mode, that's a different story.
Is this usage even supported?

That's definitely out of scope. What I was pointing at was more along the lines of : theoretically our own code could start using the dataSource before the migration is done in a controlled way... thereby doing it in an entirely uncontrolled way, without backups or checks.

I'm not sure what logic Spring uses to start dependencies, but the chain here looks like this:

dataSource -> daoHelper
           -> liquibase

If somehow Spring decides to switch the order, then we're in trouble.

The order looks solid to me, but it should be reviewed by someone who has deep knowledge about the internals of airsonic and Spring. :)
Apologies for the noise!

I have been looking forward to this very much.

However, there are currently several PR related to DB(or DB testing).
Not a bad sign. I think all of them are important.
But happy puzzle.

@fxthomas
In reality there will be an order in the application of PR, but do you have consensus or story among the major members of Airsonic?

#1340, #1358, #1364, #1367, #1368, #1369, #1363 , and this PR.

If the order is simply shown, the gaze of the committers may be concentrated.

@tesshucom A nice puzzle indeed, although I don't mind rebasing this -- it is part of development! 😄

I initially set the milestone for this to 11.0, so that the next major release could be the "big upgrade", but this was not really the result of a consensus.

Ideally I'd like for this and Spring Boot 2 to be done on the same release. Since these 2 are relatively risky upgrades it would make sense to get feedback on both at the same time, and finalize the release once we have a stable release candidate.

What do you think of this plan?

Well, I'm not dissatisfied with any plan. 😉

What we know is that if a scenario is shown to some extent, there will probably be more committers that will preferentially collaborate on high priority topics.
Otherwise, the interest of the third party will fade.

A series of PR is an engineer's concern and can be useful later.
It will also attract engineers who are used to newer libraries, so ivery benefitable.

But #1340 is a problem for some users, and there are people who are in trouble now.
If anything, I feel #1340 is different category.

randomnicode mentions Hikari.
So if thread pooling improvements are part of muff1nman's plan, It is recommended that you resolve and merge early.
At that time, verification of #1340 is necessary.
(#1340 should have many partners. In various environments.)

Or assuming there is a problem with the current implementation, #1340 is declared as a block item.
We can't move on until we fix the bugs together.

Anyway, by closing what is definitely a high priority, the number of puzzle pieces is reduced.

Sorry for commenting on this pr a bit late. I've commented on the relevant code sections.

Aside from that, here are my general thoughts on the PR:

• I don't think there are any concerns regarding using the datasource while migrations are happening. Aside from liquibase, the AbstractDAO is the parent class for all DAOs, and since AbstractDAO needs the daoHelper injected, no Dao bean may be instantiated while the daoHelper is still going through its instantiation. The only concern here would be some class injecting/autowiring the JDBCTemplate, NamedJdbcTemplate or DataSource directly without injecting DaoHelper, and you can easily check for references to those classes and see where they end up. Liquibase, I can see has already been made dependent on daoHelper.

• A concern I do have is regarding the timing of the backup copy operation. Your threadpool might already have instantiated a connection to the db by the time the daoHelper is going through its postconstruct and be in the process of upgrading it when we start to copy over files. We're probably just getting lucky, I suspect DBCP2's initial pool size is 0, and spring isn't overriding it. Other threadpools might not be so forgiving with defaults, or you might get a user who's overriding the initial pool size in his properties. Additionally, your db is live while you're copying files over. That might be an issue. So, the best place to do this copy, in my opinion, is probably in the Application prior to running the Application. That ensures the DB pool hasn't been instantiated.

• I do think the one particular class (LegacyHsqlHelperDao) is too extrapolated. In my opinion, most of these checks are too drawn out. All we're looking to do is, if there is an upgrade, make a backup copy before it and execute a (shutdown) command after it. Everything else is just logging or fluff. We can't stop the upgrade, we're just looking to save the state before it and consolidate after it:

  • if db is hsqldb and db version mismatch with driver
    • make a backup copy
    • shutdown db

That's about it. All the other checks about presence or absence of files are redundant in my opinion, and it's not like we do anything anyway about them. The connection pool will establish anyway no matter what we return from those checks. Just get out of the way and let the driver do it's thing (which we do anyway at the end of the day).

So, in short, I would restructure the PR:

• In Application: Read the properties and the db driver and copy files if necessary, and set an env variable to do a db shutdown later. Fun fact, you'll have to figure out what profile you're running to see what url to use, because it might not be legacy... you might just decide to copy and backup anyway if there's a file at that location, whether its a used db or not (edited for this scenario)...

public class Application {
...
private static void doConfigure(...) {
  ...
  if (getDefaultDBLocation().exists() && getDefaultDBDriverVersion() != getDefaultDBVersion()) {
    logger.warn("backing up default db...");
    try {
       getFileUtils.copyDirectory(getDefaultDBLocation(), getDefaultDBBackupLocation());
    } catch (Exception e) { logger.warn("couldn't backup db", e); }
    System.setProperty("shutdownDbAtStart", "shutdown");
    logger.warn("done with db backup...");
  }

• In LegacyHSqlDaoHelper: In postconstruct, just shutdown the db if env variable is set

if ( StringUtils.equals(System.getProperty("shutdownDbAtStart"), "shutdown") {
  getJdbcTemplate().execute("SHUTDOWN SCRIPT");
  getJdbcTemplate().query*(get db version);
  //log db version
}

That should be all you need.
Hope this helps?

One more note, regarding landing the PRs:

• This PR is definitely a non-backwards compatible breaking change. It will need a major revision change.
• The rest of the PRs quoted above by @tesshucom are not breaking changes. They're significant infrastructural changes for sure, but none of them are breaking API changes. By semver, they don't need a major version increment. I would probably land them and increment maybe the minor version for enhanced functionality, but definitely not the major version.
• As such, the order matters. I would land the minor semver changes, and then do the switch to the major upgrade. As such, people in the previous release who don't want to upgrade to the next major revision can also enjoy the benefits of an upgraded platform without worrying about backwards-compability issues.

So, aside from PR comments:

A concern I do have is regarding the timing of the backup copy operation.

Me too.

So, the best place to do this copy, in my opinion, is probably in the Application prior to running the Application.

That may actually be a good idea. I'm going to restructure the PR in order to do this.

In my opinion, most of these checks are too drawn out.

I don't trust the HSQLDB upgrade process, so I'd rather check my assumptions at each step. The checks are not there for the sole purpose of checking, they're here to warn users that something may have been wrong, instead of finding it out much later after they removed the backup.

@airsonic/maintainers will merge that this week-end!