DBManul Merge Discussion

These were the questions and comments I had from going through all the commits. Once we get them figured out we can finish the merge into liquibase master. NOTE: I’m going to be away from my computer for the next couple days.

Logging

You made some changes to how the logging works, introducing TreeStyleLogger and adding an SQL level. You also made some changes to what goes to STDERR and STDOUT. They are good changes, but one thing I’ve had on the list for Liquibase 3.6 has been a switch to slf4j (I used it in Liquibase 4). 

If we use a mores standard logger, the sql level is probably not something we can add, but I do want the logger to be able to tell SQL statements from non-sql statements. I think there are ways in slf4j that we can do that, but haven’t gotten a chance to research the best options yet. 

I didn’t look at your TreeStyleLogger output, but wondering if the MDC data in SLF4j would be capturing what the tree-style is adding while still being cross-logger-compatible? If TreeStyleLogger is more about formatting the output, will it still work with SLF4j?

One thing the current Liquibase use of stderr and stdout does is make sure that if you are running updateSql you can pipe that directly into your sql prompt, unix-style. I’m thinking your switching with stderr and stdout may have broken that functionality. Did it? 

The way logging vs. command output works in general needs to support that pipe use case as well. Part of the switch in the logging system was going to be a better distinction between command-line output vs. logging. 

Except for needing to look harder at how stderr and stdout work for pipe, I’d be OK merging the logging changes into master, but would like to make the switch to slf4j in 3.6 so what is in there would be refactored.

Somewhat logging-related is the adding of the banner in the command line output. It can be helpful, but also gets in the way of pipe-ing output so I’d tend to say it’s not worth adding. Adding the build info to the help docs or a separate command would be useful, though.

Also somewhat logging related, 7e30edd took out catching Throwable, but there are places where Throwables as thrown in jdbc drivers. etc and we like to catch that to provide better context around the exception.

Line Ending Standardization

There were a couple changes you put in around handling of line endings. There was a bug that I’ve fixed recently but may not have made it into the main repo yet that may also fix some of that. I’ll see if that change also addresses some of the changes you did. I think all the checksuming should be line-ending independent.

Database Version Support

You cut off support for SQL Server prior to 2008 which I think is fine, that is very old. 

However, You took out support for for pre-9.2 postgres but I didn’t see a lot of great reasons to need to. That is still fairly old, but probably not so old that we can drop support for it unless there is a great reason. Why were you thinking it should be dropped?

I18N Support

While I like the idea of localizing Liquibase, when I’ve tried in the past I run into problems keeping everything in sync. When I make a change or get a pull request with a change, just the english version would be updated and I need to wait on non-english speakers to be able to catch things up which can be difficult to impossible and in the end the non-english documentation and prompts were out date which tends to be as bad as non-localized.

However, that’s my view as an english-native speaker :slight_smile: How important and useful do you feel localization is vs. the overhead of maintaining it?

Misc

a86a4345 added logic that better handles the changeSet’s context expressions in the databasechangelog table, but it duplicates the logic a bit. I think the logic should actually just go in the ContextExpression.toString() method

Compatibility-Breaking Changes

6675e15 removed the shouldAddPrimaryKey method, but that is used by extensions and so needs to stay

23809c1d switches includeCatalog, includeSchema, and includeTablespace to be true by default which will cause problems for people expecting the old version and so need to stay the old way

c4b9a9c7 makes the Loader not skip classes with $ signs in them, but I’d be worried that people may have their own extensions which include inner-class defined services which would have a $ in the name. Note: for Liquibase 4 I’m using the Java ServiceLoader support and will probably backport that in 3.7.

7d79ee72 removed the canCacheLiquibaseInfo method, but I believe that is also used by extensions and so needs to stay

5fbec083 removed Database.correctSchema which is a deprecated method, but removing the method will also break extensions with @Override so I think it’s better to just leave in for now

Why did you take out the null check in 1e32c349?

613r6s30 takes out bracketing of datatypes in mssql, but I seem to remember that there was a pull request that explicitly had them added because they should always be bracketed. I’ll have to see if I can find why that was.

Nathan

Also wanted to point out that the above lists a few things, but it’s really not that much and there is a ton of great stuff in the branch. Thanks again for all the work you did, especially around cleaning up the automated tests that I’ve let languish for far too long. 

Nathan

I remember you mentioning the Ant issue, Daniel. My thought has been that slf4j is doing the same log indirection layer that we currently have/want, but in a more “standard” way. No use re-implementing something that works just fine. I liked the fact that the slf4j API can be bound to a variety of different logging systems depending on what the user wants to use. Commons-logging is a similar idea as well, but slf4j seemed to have some newer features that were nice. 

I also don’t mind if the CLI is tightly bound to the the logback slf4j implementation because I see that as more as a stand-alone tool that is ran independently and can have dependencies that are not in the core liquibase library, such as commons-cli and logback. Especially in Liquibase 4, I’m trying to build in a central concept of common “commands” which can be called by the CLI, Ant, Maven, etc. so that the CLI can truly be a stand-alone thin wrapper that does what it wants without affecting anything else.

In the end, however, I think what we need is an API that lets us do a few things:

  1. Plug into any logging implementation (java.util logging, log4j, etc.) so the Liquibase logs can be included in the rest of the application's logs
  2. Be able to provide a context (such as the current changeset, object being snapshotted, etc) that a log was created in
  3. Differentiate between DEBUG, INFO, WARN, SEVERE, and FATAL levels
  4. Capture executed SQL in a distinct channel
  5. Capture query SQL separately from "update" SQL
  6. Capture user error messages
  7. Capture system errors and stacktraces
  8. Capture "progress" messages like "executed changeSet 8 of 91"

What makes it difficult is that the goal is to have the vast majority of the Liquibase code the same whether it is ran through Ant or Maven or Spring or the CLI, which means the the business logic will need to be generating all that information without knowing how it will be used. It should be up to the end-point integration to be able to use the logging info as it needs. Ant, Maven and Spring would just log out whatever matches the log level. The CLI, on the other hand, is more complex because it would normally output just progress messages and user error messages, but for updateSQL, it should just output the “update” sql, and the user should be have it output stacktraces or additional logging if need be. 

Perhaps that is all more than SLF4j or any other existing generic-logging system can already do on it’s own. I was still experimenting a bit with how it could work in Liquibase 4, and once something got figured out there I would backport it back to 3.x as possible. I would maybe need to leverage the Command classes in addition to the logging system to get everything we need. Or maybe just still keep our own custom logging system instead of trying to write to someone else’s system. And, maybe the Ant problem is a showstopper for slf4j in general. 

So… to connect back to the Logging changes in the merge… I definitely agree that there needs to be changes to how logging is handled, but I think it is part of a larger change we need to make so I’m worried about pulling a partial change in 3.6 when we may make larger changes again soon. I think if we bring the changes in as they are, I think it would force us to work through the larger logging changes before we could release your changes. Or, we could pull the logging changes into a separate branch and look at the larger changes with 3.7. 

Given that I was hoping to figure out better logging in 3.6 anyway, I think it is best to leave your changes in, but they may be refactored as part of a more complete logging change in 3.6. As part of that, we can make sure the pipe support still works and how throwables are handled. 

Nathan

 

I started a logging-related thread at http://forum.liquibase.org/#Topic/49382000001706025

Nathan

I went through the “incompatible changes” above:

- Re-introduced sholdlAddPrimaryKey- Switched includeSchema/includeTablespace/includeCatalog default back to "false" - Re-allowed service classses with $ in them, but exclude anonymous and synthetic classes - Re-introduced database.setCanCacheLiquibaseTableInfo - Re-introduced database.correctSchema
  • I decided to leave the change to the sqlserver escaping in. I think CORE-2217 made it escape everything which doesn’t really match the rest of the liquibase logic

Nathan

I also:

  • put back support for postgres 9.2 and earlier

  • looked at your context logging handling. I see more what you are doing there, so I’ll leave the code as is.

  • I left the change to path handling in Nathan

Hi Nathan,

I used to have a closer look at the logging part some time ago, but unfortunately couldn’t finish any work on it.

There where some problems when migrating the codebase to slf4j. The ant integration (and maybe maven too) is problematic as the Logger is initialized by the ant runtime and is given to the process. I did not see how to access ant logging in a static way.

In Liquibase 4 the ant integration was missing (that why you may have not run into that).

I also had a look on other tools (like Flyway) and they do not use the Logging API directly but keep a redirection layer, the same way as you currently do, to solve exactly this problem.

Another problem of Liquibase 4 is that the CLI log-level switch is strongly coupled to logback. This is the kind of dependency slf4j tries to break. A clean solution should be to drop the log-level CLI option entirely and leave the configuration to the files read by a concrete logging implementation (e.g. logback.xml, log4j.properties…).

kind regards

Daniel

I pushed the dbmanul merge branch to master. I will continue to do testing as I bring in more pull requests and fixes to prepare for a 3.6 release.

Let me know if you run into any problems.

Nathan