Hey @filipelautert! Hope you have been well!

I created this PR in draft because I'd like some help/opinions here. I've got the majority of this working, but can't figure out how to get the correct DEPLOYMENT_ID.

It appears that the properties are all replaced in changelog files before a lock is accquired and the update action is actually run. When the update run is started, accquiring the change lock resets the deployment ID (as explained by this comment.

So my question is... any thoughts on how I could achieve getting the deployment ID so it is available to the changes?

I'm not hard set on the usage of the properties for this... that is just the first thing that came to mind as that substitution is already built in to Liquibase. Not sure if you or your team mates could think of another mechanism that would be better suited?

Thanks!

So my question is... any thoughts on how I could achieve getting the deployment ID so it is available to the changes?

Hey @jasonlyle88 ! Could you provide an example changelog were you would use the deployment id?

But you are right.. we reset the deployment id with each reset. And that's like this because it didn't matter until now as the deployment id is the same for all the operations executed in a single update .
I'm thinking what would be the implications of creating one deployment id and never resetting it, especially for multitenant environments. In this case you would have the same id in different databases.. maybe something else would change? Or maybe flows would be affected too? Also we can't remove the resets as they are important in multithread environments.

OR maybe it would be easier to parse the deployment id parameter after the code comment you mentioned .

Hey,

Here is an example of how I was thinking this could be used

<?xml version="1.0" encoding="UTF-8"?>
<databaseChangeLog 
    xmlns="http://www.liquibase.org/xml/ns/dbchangelog" 
    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" 
    xmlns:ora="http://www.oracle.com/xml/ns/dbchangelog-ext" 
    xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-4.17.xsd"
>
    <changeSet
        id="example"
        author="jlyle"
    >
        <sql endDelimiter="/">
            begin
                -- Do installation stuff
                ...
            exception when others
                --ERROR HANDLER
                logger.log_error(
                    p_text => 'Error with liquibase run',
                    p_additional_info => t_info_tab(
                        t_info_row('Error Code',        sqlcode),
                        t_info_row('Error Message',     sqlerrm),
                        t_info_row('Deployment ID',     '${LIQUIBASE_EXECUTION_DEPLOYMENT_ID}'),
                        t_info_row('Changelog File',    '${LIQUIBASE_EXECUTION_CHANGELOG_FILE}'),
                        t_info_row('Changeset ID',      '${LIQUIBASE_EXECUTION_CHANGESET_ID}'),
                        t_info_row('Changeset Author',  '${LIQUIBASE_EXECUTION_CHANGESET_AUTHOR}')
                    )
                );
            end;
            /
        </sql>
    </changeSet>
</databaseChangeLog>

I agree only having a singular deployment ID could be problematic. Technically, 10 liquibase updates could be executed at almost the same time and end up with the same deployment ID (since it is time based). But only the first one to get the lock executes and the deployment ID is recalculated at that time. This would distinguish each update attempt because its deployment ID time is based on when it got the lock, not when it was executed. Basically, I agree that the resets are important.

I would be interested in how you think the deployment ID could be parsed after the lock is obtained because that would definitely solve the problem. Are you thinking move all parameter replacements to after the database lock is established? Or somehow just doing it for this one special case?

I suppose we could have a singular deployment ID that does not get reset if we make it more unique. To that end, I did a quick test with the java UUID:

import java.util.UUID;
import java.math.BigInteger;

class UUIDToInt {
    // Shows that the UUID theoretically can span anything from 1 digit to 39 digits
    public static void main(String[] args) {
        UUID uuid;
        String uuidString;
        BigInteger bi;
        String integerString;
        
        int shortestLength = 99999999;
        int integerLength;
        
        
        for (int i = 0; i < 30; i++) {
            uuid = UUID.randomUUID();
            uuidString = uuid.toString().replaceAll( "-" , "" );
            if (i == 0) {
                // Test smallest possible number
                uuidString="00000000000000000000000000000000";
            }
            else if (i == 1) {
                // Test largest possible number
                uuidString="ffffffffffffffffffffffffffffffff";
            }
            bi = new BigInteger( uuidString , 16 );
            integerString = bi.toString() ;
            integerLength = integerString.length();
            
            if (i > 1 && integerLength < shortestLength) {
                shortestLength = integerLength;
            }
            
            System.out.println(integerLength + ": " + integerString);
        }
        
        System.out.println("Shortest generated length: " + shortestLength);
    }
}

This shows that theoretically a generated integer from UUID can be anywhere from 1-39 digits long. Although, practically it will most likely be 36+ digits long from my testing.

If it is acceptable to convert the deployment ID to a UUID string (always 36 characters long, 32 without dashes) or a UUID numeric representation (0-39 characters long), that would allow a single deployment ID to be generated and not need to be regenerated ever since it is unique.

For reference, the deployment ID column is currently a character column of length 10.

One difference in the UUID based number approach that should be noted is that if N values are generated, the Nth + 1 value is not guaranteed to be greater than the Nth value.

Right now, since the DeploymentID is time based, the next generation is always a greater (assuming that it is generated at a different time, which is guaranteed by the database lock)

Hey,

Here is an example of how I was thinking this could be used

<?xml version="1.0" encoding="UTF-8"?>
<databaseChangeLog 
    xmlns="http://www.liquibase.org/xml/ns/dbchangelog" 
    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" 
    xmlns:ora="http://www.oracle.com/xml/ns/dbchangelog-ext" 
    xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-4.17.xsd"
>
    <changeSet
        id="example"
        author="jlyle"
    >
        <sql endDelimiter="/">
            begin
                -- Do installation stuff
                ...
            exception when others
                --ERROR HANDLER
                logger.log_error(
                    p_text => 'Error with liquibase run',
                    p_additional_info => t_info_tab(
                        t_info_row('Error Code',        sqlcode),
                        t_info_row('Error Message',     sqlerrm),
                        t_info_row('Deployment ID',     '${LIQUIBASE_EXECUTION_DEPLOYMENT_ID}'),
                        t_info_row('Changelog File',    '${LIQUIBASE_EXECUTION_CHANGELOG_FILE}'),
                        t_info_row('Changeset ID',      '${LIQUIBASE_EXECUTION_CHANGESET_ID}'),
                        t_info_row('Changeset Author',  '${LIQUIBASE_EXECUTION_CHANGESET_AUTHOR}')
                    )
                );
            end;
            /
        </sql>
    </changeSet>
</databaseChangeLog>

I agree only having a singular deployment ID could be problematic. Technically, 10 liquibase updates could be executed at almost the same time and end up with the same deployment ID (since it is time based). But only the first one to get the lock executes and the deployment ID is recalculated at that time. This would distinguish each update attempt because its deployment ID time is based on when it got the lock, not when it was executed. Basically, I agree that the resets are important.

I would be interested in how you think the deployment ID could be parsed after the lock is obtained because that would definitely solve the problem. Are you thinking move all parameter replacements to after the database lock is established? Or somehow just doing it for this one special case?

Right, that's the idea! I discussed with some people here and we agreed that if it is possible to move the replacement code inside AbstractUpdateCommandStep it would be great (I'm not sure if it would work). Otherwise keep the parameter replacement where is it, but if it finds LIQUIBASE_EXECUTION_DEPLOYMENT_ID then flag it to be replaced after the last lock is acquired.

What do you think?

I'm absolutely up for either of those items. I haven't dug to much into the AbstractUpdateCommandStep part of things outside knowing its the driver of this. With moving all property replacements, I realize this is a pretty big change. Is that something you or another member want to jump in to handle to make sure there are not any consequences I'm not noticing? Or do you want me to go ahead and attempt it and just report on my attempts?

I tried digging into this... and it requires a lot of knowledge about how everything fits together. I'm a little lost at how I could successfully move the replacement of the properties to AbstractUpdateCommandStep. I would definitely appreciate help here.

Also curious to hear feedback about having a more static deployment ID as I discussed above with the UUID/ numeric version of UUID that does not change with the resets!

Okay... I realized another facet of this last night. Continuing down this path, the replacement of the deployment ID is being done before a checksum is calculated for a changeset. This means that a changeset using this property will always appear as having changed since the changeset would be different because the deployment ID will be different on every execution. This is obviously not ideal (runOnce changesets with this property will cause errors on every run other than the first run and runOnChange changesets will run every single time).

So, it may actually be preferable to replace the deployment ID property (and maybe the other properties I am introducing as well?) later in the process (after the checksums have been calculated and after the lock has been acquired/final deployment ID is generated). Do you agree?

Is there a mechanism in place already to do property replacements later on in the update process?

@filipelautert Okay... sorry for all the pings!

But I think I have a viable solution. Instead of moving the replacement code later in the overall command process (I couldn't see a clean way to do this without A LOT of refactoring), I instead moved the lock acquistion code sooner in the overall command process.

I did this by altering the DbUrlConnectionCommandStep to acquire a changelog lock as soon as a connection to the database is established (and generate a deployment ID so it is available). You'll notice in the DbUrlConnectionCommandStep that I include this lock acquisition for 4 classes. This is because those are the four classes that call the waitForLock method. If there is a better/cleaner way of determining when to acquire the changelog lock and when not to at this step, I'm all for changing this method!

I ran the full test suite and had to update a couple existing tests because of the logic moving around, but overall this seems to be a pretty small change in the grand scheme of things.

Please take a look and let me know if this is viable or not!

I also haven't provided any new tests... want to make sure this process is accepted for trying to write tests (if there are any tests you want me to write...)

It may be cleaner to add a LockServiceCommandStep step to the update command's pipeline right after the DbUrlConnectionCommandStep step... but I can't figure out how to add a step to a pipeline like that! Happy to rework this to implement that if you or someone would be able to help me there!

@filipelautert Okay, being very chatty, sorry about that. Tried one more thing: setting LIQUIBASE_HOME to a non-existent directory. This also allowed the IDE to run the CLI without issues. So it seems LIQUIBASE_HOME needs to be set... but it doesn't need to be set to anything specific. I assume this would need to be set to something specific if you need special database drivers or custom things included from the lib or internal/lib directories. But I imagine that is a special case. SO, seems like doc just needs to be updated to state that it needs to be set, but only needs to point to a directory that contains lib or internal/lib if files need loaded.
Thoughts?

Hi @jasonlyle88,

Sorry for my late reply, I was just following this thread. LIQUIBASE_HOME should always be set if you are using CLI it would use Liquibase from that directory, now if you use it from a development environment (IDE) as you are doing using LiquibaseLuncher, then its value will be checked to validate whether it's null or "" string.

Similarly to @filipelautert in my working directory I don't have core and commercial jars in the internal/lib directory.

Regarding testing, something I forgot to mention the other day is when running mvn clean install your liquibase.sdk.local yml file, needs to be set to run the default integration test that is H2, if you try having any other db set there integration test execution will fail, hence your build will do it as well. I only change that value when I want to run integration tests for any other specific DB, but not when building.

Thanks, Daniel.

Hey @MalloD12 ,

I don't have a liquibase.sdk.local.yml file. I never have had one, and previously mvn clean install would work find. But it fails since 5619

@filipelautert Okay, being very chatty, sorry about that. Tried one more thing: setting LIQUIBASE_HOME to a non-existent directory. This also allowed the IDE to run the CLI without issues. So it seems LIQUIBASE_HOME needs to be set... but it doesn't need to be set to anything specific. I assume this would need to be set to something specific if you need special database drivers or custom things included from the lib or internal/lib directories. But I imagine that is a special case. SO, seems like doc just needs to be updated to state that it needs to be set, but only needs to point to a directory that contains lib or internal/lib if files need loaded.
Thoughts?

Hi @jasonlyle88,
Sorry for my late reply, I was just following this thread. LIQUIBASE_HOME should always be set if you are using CLI it would use Liquibase from that directory, now if you use it from a development environment (IDE) as you are doing using LiquibaseLuncher, then its value will be checked to validate whether it's null or "" string.
Similarly to @filipelautert in my working directory I don't have core and commercial jars in the internal/lib directory.
Regarding testing, something I forgot to mention the other day is when running mvn clean install your liquibase.sdk.local yml file, needs to be set to run the default integration test that is H2, if you try having any other db set there integration test execution will fail, hence your build will do it as well. I only change that value when I want to run integration tests for any other specific DB, but not when building.
Thanks, Daniel.

Hey @MalloD12 ,

I don't have a liquibase.sdk.local.yml file. I never have had one, and previously mvn clean install would work find. But it fails since 5619

Ahh ok, that file liquibase.sdk.local.yml is mostly needed when trying to run integration tests locally, if that's not your case you are find without it.

@filipelautert Okay, being very chatty, sorry about that. Tried one more thing: setting LIQUIBASE_HOME to a non-existent directory. This also allowed the IDE to run the CLI without issues. So it seems LIQUIBASE_HOME needs to be set... but it doesn't need to be set to anything specific. I assume this would need to be set to something specific if you need special database drivers or custom things included from the lib or internal/lib directories. But I imagine that is a special case. SO, seems like doc just needs to be updated to state that it needs to be set, but only needs to point to a directory that contains lib or internal/lib if files need loaded.
Thoughts?

Hi @jasonlyle88,
Sorry for my late reply, I was just following this thread. LIQUIBASE_HOME should always be set if you are using CLI it would use Liquibase from that directory, now if you use it from a development environment (IDE) as you are doing using LiquibaseLuncher, then its value will be checked to validate whether it's null or "" string.
Similarly to @filipelautert in my working directory I don't have core and commercial jars in the internal/lib directory.
Regarding testing, something I forgot to mention the other day is when running mvn clean install your liquibase.sdk.local yml file, needs to be set to run the default integration test that is H2, if you try having any other db set there integration test execution will fail, hence your build will do it as well. I only change that value when I want to run integration tests for any other specific DB, but not when building.
Thanks, Daniel.

Hey @MalloD12 ,
I don't have a liquibase.sdk.local.yml file. I never have had one, and previously mvn clean install would work find. But it fails since 5619

Ahh ok, that file liquibase.sdk.local.yml is mostly needed when trying to run integration tests locally, if that's not your case you are find without it.

So this is still an issue. In the docs for how a developer should get started (https://contribute.liquibase.com/code/get-started/env-setup/) it basically says pull the repo and then run mvn clean install. That fails (and is still failing for me) on the hsqldb integration tests. Could be quite the hurdle for new people coming trying to contribute (or even people like me who have been contributing for a while and just aren't super Java literate).

@filipelautert Okay, being very chatty, sorry about that. Tried one more thing: setting LIQUIBASE_HOME to a non-existent directory. This also allowed the IDE to run the CLI without issues. So it seems LIQUIBASE_HOME needs to be set... but it doesn't need to be set to anything specific. I assume this would need to be set to something specific if you need special database drivers or custom things included from the lib or internal/lib directories. But I imagine that is a special case. SO, seems like doc just needs to be updated to state that it needs to be set, but only needs to point to a directory that contains lib or internal/lib if files need loaded.
Thoughts?

Hi @jasonlyle88,
Sorry for my late reply, I was just following this thread. LIQUIBASE_HOME should always be set if you are using CLI it would use Liquibase from that directory, now if you use it from a development environment (IDE) as you are doing using LiquibaseLuncher, then its value will be checked to validate whether it's null or "" string.
Similarly to @filipelautert in my working directory I don't have core and commercial jars in the internal/lib directory.
Regarding testing, something I forgot to mention the other day is when running mvn clean install your liquibase.sdk.local yml file, needs to be set to run the default integration test that is H2, if you try having any other db set there integration test execution will fail, hence your build will do it as well. I only change that value when I want to run integration tests for any other specific DB, but not when building.
Thanks, Daniel.

Hey @MalloD12 ,
I don't have a liquibase.sdk.local.yml file. I never have had one, and previously mvn clean install would work find. But it fails since 5619

Ahh ok, that file liquibase.sdk.local.yml is mostly needed when trying to run integration tests locally, if that's not your case you are find without it.

So this is still an issue. In the docs for how a developer should get started (https://contribute.liquibase.com/code/get-started/env-setup/) it basically says pull the repo and then run mvn clean install. That fails (and is still failing for me) on the hsqldb integration tests. Could be quite the hurdle for new people coming trying to contribute (or even people like me who have been contributing for a while and just aren't super Java literate).

Then, I would recommend that you create a separate issue for that and then we can ask the docs team to prioritize it.

Thanks,
Daniel.

@filipelautert As this is ready for review, would you mind removing the awaiting_response label? Also, do you need anything else from me here to begin this review? I'd love to get this going so I can also get going on its dependent PR #5653

@rberezen Looks like this has been approved for 2 weeks, anything I need to know about as to why it hasn't been merged yet? Just want to make sure I'm not missing anything!

@rberezen Looks like this has been approved for 2 weeks, anything I need to know about as to why it hasn't been merged yet? Just want to make sure I'm not missing anything!

Sorry for the delay. The changes have been merged. Thank you for your contributions!

No problem, thank you for everything you all do here!