Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DefaultTaskConfigurer constructors should use TaskProperties.getTablePrefix() instead of constant TaskProperties.DEFAULT_TABLE_PREFIX #939

Open
nightswimmings opened this issue Dec 22, 2023 · 8 comments
Milestone

Comments

@nightswimmings
Copy link

nightswimmings commented Dec 22, 2023

Right now if one uses DefaultTaskConfigurer without specifying a prefix, it looses the ability to use the one from properties.
I think when not passed as a param, it should use TaskProperties.getTablePrefix() and instead set the default at the TaskProperties level

Besides, shouldn't "Class.forName("javax.persistence.EntityManager");" be now "Class.forName("jakarta.persistence.EntityManager");" considering this corresponds to the release train for Boot 3.2?

@cppwfs
Copy link
Collaborator

cppwfs commented Jan 2, 2024

Thank you for the comment and good catch on the javax.
But, I'm a little confused, as to why we'd want to pass in the TaskProperties to obtain the prefix when other constructors allow for the setting of the prefix?

@nightswimmings
Copy link
Author

nightswimmings commented Jan 3, 2024

Because if we want to customize only one argument then we don't have to deal with and research internal mechanisms for table prefix
I mean I think it makes sense to assume that as long as you don't pass the tablePrefix as argument, you want to benefit from the default properties mechanism

Think somebody that just wants to pass a custom datasource, then he performs

public CustomTaskConfigurer(@BatchDataSource DataSource dataSource) {
        super(dataSource); 
}

But then he realises that the tables are being created in another place (when using latest SpringBoot3-enabled SCDF, for instance), he or she needs to research the implementation and understand that you need to reproduce the previous default

public CustomTaskConfigurer(@BatchDataSource DataSource dataSource, TaskProperties taskProperties) {
        super(dataSource, taskProperties.getTablePrefix(), null); 
    }

It's an undersirable side-effect, because you are extending DefaultTaskConfigurer, so everything yoo don't explicitly touch, should remain as the Default configuration according to the name

Indeed ideally IMO it should be something like

(@EnabledConfigurationProperties TaskProperties) ? taskProperties.getTablePrefix() : DEFAULT_TABLE_PREFIX

@favna
Copy link

favna commented Apr 26, 2024

I'm facing this same issue now where we provide a custom DataSource and then I get this error:

Caused by: java.lang.IllegalArgumentException: Invalid TaskExecution, ID 12 not found

Our custom TaskConfigurer is:

@Component
@Profile("!springBootTest & !commandline")
class TaskConfigurer extends DefaultTaskConfigurer {
	public TaskConfigurer(DataSourceWrapper dataSourceWrapper) {
		super(dataSourceWrapper.getDataSource());
	}
}

and the relevant other classes are:

import com.zaxxer.hikari.HikariDataSource;
import org.springframework.beans.factory.annotation.Qualifier;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.boot.autoconfigure.jdbc.DataSourceProperties;
import org.springframework.context.annotation.Profile;
import org.springframework.stereotype.Component;

@Component
@lombok.Value
@Profile("!springBootTest & !commandline")
class DataSourceWrapper implements AutoCloseable {
	HikariDataSource dataSource;

	DataSourceWrapper(@Value("${spring.datasource.hikari.maximum-pool-size:10}") int maxPoolSize,
	                  @Value("${spring.datasource.hikari.minimum-idle:1}") int minimumIdle,
	                  @Value("${spring.datasource.hikari.idle-timeout:30000}") int idleTimeout,
	                  @Qualifier("dataflowDataSourceProperties") DataSourceProperties dataSourceProperties) {
		var hikariDataSource = dataSourceProperties.initializeDataSourceBuilder()
				.type(HikariDataSource.class)
				.build();

		hikariDataSource.setMaximumPoolSize(maxPoolSize);
		hikariDataSource.setMinimumIdle(minimumIdle);
		hikariDataSource.setMinimumIdle(idleTimeout);

		this.dataSource = hikariDataSource;
	}

	@Override
	public void close() {
		this.dataSource.close();
	}
}
import org.springframework.boot.autoconfigure.jdbc.DataSourceProperties;
import org.springframework.boot.context.properties.ConfigurationProperties;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.context.annotation.Profile;

@Configuration
@Profile("!springBootTest")
@ConfigurationProperties("spring.datasource")
public class DataSourceConfig {
	@Bean
	public DataSourceProperties dataflowDataSourceProperties() {
		return new DataSourceProperties();
	}
}

I am unsure if there is a way to work around this issue at the moment, but if not then it is a major blocker for our upgrades to using boot3 mode in SCDF 2.11.2. The only constructor for DefaultTaskConfigurer that I can see that allows overriding both DataSource and tablePrefix is

public DefaultTaskConfigurer(DataSource dataSource, String tablePrefix, ApplicationContext context)

but then I would have no idea what to provide for ApplicationContext nor if I can even reliable get the desired tablePrefix in my custom TaskConfigurer.

We use Pivotal CloudFoundry and in the User provided environment variables I do see that a boot3 related tablePrefix gets assigned:
ishare-1714141356

@cppwfs
Copy link
Collaborator

cppwfs commented Apr 26, 2024

Let's see If we can squeeze this into 3.1.2

@cppwfs cppwfs added this to the 3.1.2 milestone Apr 26, 2024
@cppwfs
Copy link
Collaborator

cppwfs commented May 1, 2024

Since this is a change in behavior this will be in the 3.2 release of Task.

@cppwfs cppwfs modified the milestones: 3.1.2, 3.2 May 1, 2024
@favna
Copy link

favna commented May 1, 2024

Can you provide any rough estimation whatsoever for when that may be released so I can update our Jira with more information? For now, the story is flagged in the current sprint (2 weeks remaining for it) but if 3.2 won't be coming for some time I'd rather push it back to the backlog.

@cppwfs
Copy link
Collaborator

cppwfs commented May 2, 2024

I'm looking at giving the user the ability to pass in a TaskProperties into the constructor and then letting the DefaultTaskConfiguration constructor resolve tablePrefix as @nightswimmings suggested above. Since this solution doesn't affect the default behavior I can put it back onto the 3.1.2 release.

@cppwfs
Copy link
Collaborator

cppwfs commented May 30, 2024

This has been pushed to 3.2, based on discussions that a change like this can't be put into a patch release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants