Spring SqlServerMaxValueIncrementer variant based on MS SQL sequences (avoiding deadlocks) [SPR-16886]

Tyler K Van Gorder opened SPR-16886 and commented

We are currently refactoring a legacy scheduler application to use spring cloud data flow and spring cloud task. The tasks are mostly spring batch applications and we have been encountering deadlock issues in the batch meta tables when using MS SQL Server.

Digging into this a bit further, we discovered our issue related to the the strategy being used for sequences and there is already an issue in Spring Batch.

The use of a sequence table (likely because older versions of SQL Server didn't have support for sequences) and the strategy to delete records from that table result in deadlocks.

We were able to work around this issue by customizing the creation of the Job repository such that a custom Incrementer is installed that uses sequences instead.

So, why are we filing this issue here?

We realized we are likely to have the same issue in Spring Cloud Task's meta-tables. We had to do some more Spring gymnastics to override the Incrementer there, it was not as easy as Spring Batch, but we managed.

We also need to make sure Spring Cloud Data Flow is using the same incrementer and that is even harder to override......

All of these issues lead back to need for a better strategy for getting max values when using MS SQL Server.

Affects: 4.3.17, 5.0.6

Comment From: spring-projects-issues

Juergen Hoeller commented

What does your custom incrementer look like? Any chance you could submit its code for consideration here?

Comment From: spring-projects-issues

Tyler K Van Gorder commented

There is not much to the incrementer, as you did most of the hard work in the AbstractSequenceMaxValueIncrementer:

public class CustomSqlServerMaxValueIncrementer extends AbstractSequenceMaxValueIncrementer {


    public CustomSqlServerMaxValueIncrementer(DataSource dataSource, String incrementerName) {
        super(dataSource, incrementerName);
    }


    @Override
    protected String getSequenceQuery() {
        return "select NEXT VALUE for " + getIncrementerName();
    }
}

To get this working:

1) We had to manually change the scripts for batch and tasks to create sequences instead of tables.

2) We created the above incrementer.

In batch:

3) We had to create our JobRepository with a modified DataFieldMaxValueIncrementerFactory (to feed our new incrementer into the repository)

public class CustomDataFieldMaxValueIncrementerFactory extends DefaultDataFieldMaxValueIncrementerFactory {


    private DataSource dataSource;


    public CustomDataFieldMaxValueIncrementerFactory(DataSource dataSource) {
        super(dataSource);
        this.dataSource = dataSource;
    }


    @Override
    public DataFieldMaxValueIncrementer getIncrementer(String incrementerType, String incrementerName) {
        DatabaseType databaseType = DatabaseType.valueOf(incrementerType.toUpperCase());
        DataFieldMaxValueIncrementer dataFieldMaxValueIncrementer = null;
        if (databaseType == SQLSERVER) {
            dataFieldMaxValueIncrementer =  new CustomSqlServerMaxValueIncrementer(dataSource, incrementerName);
        } else {
            dataFieldMaxValueIncrementer = super.getIncrementer(incrementerType, incrementerName);
        }
        return dataFieldMaxValueIncrementer;
    }
}

4) Finally, we had to inject our factory into our repository via the BatchConfigurer.

We had a similar issue with Spring Cloud Task and in the data flow server we ended up overriding the TaskRepository via @Primary annotation.

It would obviously be easier if this were handled farther up the chain...but I can also see this is going to be pain:

* The framework either needs to distinguish between newer and older versions of SQL Server....and conditionally pick the correct incrementer. This would also require each downstream project to have different scripts for different variants of SQL Server AND the older versions are still going to have the deadlocking issue. YUCK. * The default incrementer for sql server could test the database to see if sequences or a table are being used and use the correct strategy. (This still doesnt solve how the down stream projects setup the initial schema) * Stick with the table-based approach and fix the dead locking, which I believe is happening when you have concurrent applications using those meta-tables. We believe the deadlocks are causes when the incrementer is cleaning up older records in the sequence table (via delete). Maybe a better option might be to have a daemon thread that wakes up periodically, starts a transaction, locks the table, deletes all but the most current value, and commits?

Thanks for your time! I know you and your team are really busy.

Comment From: spring-projects-issues

Tyler K Van Gorder commented

Hi Jeurgen,

I have a follow-up on this, I was able to isolate this issue and create a small, sandbox application that demonstrates the problem. https://github.com/tkvangorder/jdbc-sqlserver-incrementer.

Some interesting details: * To get this deadlock issue, the incrementer MUST be used within the scope of a transaction. * This does not manifest when you have a single instance of the application because the underlying incrementer has a "synchronized" keyword around the getNextKey()! However, if you run multiple, concurrent instances of the application will will see the deadlocks.

The sample project has a docker-compose file that will fire up mssql server in docker. It also has a configuration property to switch from a shared incrementer vs a unique one for each call. The deadlocks manifest pretty quickly when you turn the shared incrementer "off".

Hopefully that helps! I am going to play around with an alternate strategy for deleting the records....

Thanks

Comment From: spring-projects-issues

Tyler K Van Gorder commented

I have created a couple of variants of the incrementer that do not dead lock, along with a version that just uses the a real sequence. The increment strategy can [easily be switched via a configuration change in that project. : https://github.com/tkvangorder/jdbc-sqlserver-incrementer#strategies. I am not sure which approach you might prefer (if any), but I can submit a PR.

Thanks!

Comment From: spring-projects-issues

Tyler K Van Gorder commented

I had some time over my break to work on this and I have a PR : https://github.com/spring-projects/spring-framework/pull/2065

Comment From: tkvangorder

Looks like the migration may have assigned the wrong status to this issue. Think this should be marked as a bug because the MSSQL server incrementer strategy causes a database deadlock when two separate processes attempt to use the incrementer. Thanks, really nice to have issues on GitHub!

Comment From: snicoll

Looks like the migration may have assigned the wrong status to this issue.

@tkvangorder the migration didn't do such a thing and the related Jira issue is flagged as an enhancement (there is a link in the first comment to it)

Comment From: tkvangorder

@snicoll, Oh, you are right, I see the "type" field now...ugh.

Do you think we can reclassify this issue as a bug?

I am the one that opened this issue originally, as it manifests in Spring Batch as a critical bug here: BATCH-2147.

I also have a sandbox example that reproduces the problem (outside of the context of spring batch) here: https://github.com/tkvangorder/jdbc-sqlserver-incrementer.

And I have a pending pull request to fix it https://github.com/spring-projects/spring-framework/pull/2065

I know you and your team are crazy busy and we can wait for a fix, as we did work around the issue by overriding beans in spring cloud task, spring cloud batch, and spring cloud dataflow. It would be nice to back out those changes as we have to go over them every time we opt into a new version of those projects.

Comment From: snicoll

@tkvangorder I am not sure, the code is rather involved and it's unclear to me if that applies to every SqlServer version and/or the current incrementer was supposed to support this. I guess @jhoeller can confirm why it was categorized that way.

Comment From: tkvangorder

For Reference:

SqlServer 2008 does not have sequences and requires the use of a table with an identity column. It looks like SqlServer 2008 EOL will be July 9th, 2019 according to Microsoft's website. However, the small print on their site also states "For continued protection beyond the deadline, buy up to three years of Extended Security Updates—cover only the workloads you need while you upgrade"
SqlServer 2012+ does support sequences.

Comment From: fmbenhassine

Will this request be considered? I see it in the 6.x Backlog. It would be nice to have it in one of the milestones of SF 6.0. This would allow other projects to leverage the sequence-based incrementer from SF and avoid duplicate efforts like in Batch and Task. cc @cppwfs

Comment From: sbrannen

Closing as superseded by the following issues which have been closed.

https://github.com/spring-projects/spring-batch/issues/1448
29447

If you feel that something is missing which still needs to be addressed, please open a new issue.

Spring SqlServerMaxValueIncrementer variant based on MS SQL sequences (avoiding deadlocks) [SPR-16886]

29447