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

Introduce initialize() method in AbstractRoutingDataSource and AbstractRoutingConnectionFactory #31248

Conversation

violetbeach
Copy link
Contributor

@violetbeach violetbeach commented Sep 16, 2023

Situation is that you want to add DataSource to the DataSources of AbstractRoutingDataSource during runtime.

When i put a Data Source, I need to synchronize target Data Sources to resolved Data Sources.

At that time, I call afterPropertiesSet() as below.

@Component
public class MultiDatabaseManager {

    private final Map<Object, Object> dataSourceMap = new ConcurrentHashMap<>();

    private AbstractRoutingDataSource multiDataSource;

    public DataSource createMultiDataSource() {
        MultiDataSource multiDataSource = new MultiDataSource();
        multiDataSource.setTargetDataSources(dataSourceMap);
        multiDataSource.setDefaultTargetDataSource(defaultDataSource());
        return multiDataSource;
    }

    public void addDataSource(String hostIp, ...) throws SQLException {
        DataSource dataSource = DataSourceBuilder
                .create()
                .username(username)
                .password(password)
                .url(url)
                .driverClassName(driverClassName)
                .build();

        try (Connection c = dataSource.getConnection()) {
            dataSourceMap.put(hostIp, dataSource);
            multiDataSource.afterPropertiesSet(); // unclear intentions
        }
    }

}

However, afterPropertiesSet() is unclear.

So I suggest extract refresh() method. Then, intent can be exposed.

Please feel free to share your opinions or advice.

thanks you.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Sep 16, 2023
@sbrannen sbrannen changed the title Extract refresh() method from AbstractRoutingDataSource Introduce refresh() method in AbstractRoutingDataSource Sep 16, 2023
@sbrannen sbrannen added in: data Issues in data modules (jdbc, orm, oxm, tx) type: enhancement A general enhancement labels Sep 16, 2023
@sbrannen sbrannen self-assigned this Sep 19, 2023
@sbrannen sbrannen removed the status: waiting-for-triage An issue we've not yet triaged or decided on label Sep 19, 2023
@sbrannen sbrannen added this to the 6.1.0-RC1 milestone Sep 19, 2023
Copy link
Member

@sbrannen sbrannen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the proposal and PR.

Can you please also introduce a similar refresh() method in AbstractRoutingConnectionFactory along with a corresponding test?

@sbrannen sbrannen added the status: waiting-for-feedback We need additional information before we can continue label Sep 19, 2023
@violetbeach
Copy link
Contributor Author

Can you please also introduce a similar refresh() method in AbstractRoutingConnectionFactory along with a corresponding test?

Sure. I've done.

Please let me know if you need anything else. Thank you for your review!

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 19, 2023
@sbrannen sbrannen removed the status: feedback-provided Feedback has been provided label Sep 19, 2023
@sbrannen sbrannen changed the title Introduce refresh() method in AbstractRoutingDataSource Introduce refresh() method in AbstractRoutingDataSource and AbstractRoutingConnectionFactory Sep 19, 2023
@sbrannen sbrannen changed the title Introduce refresh() method in AbstractRoutingDataSource and AbstractRoutingConnectionFactory Introduce initialize() method in AbstractRoutingDataSource and AbstractRoutingConnectionFactory Sep 25, 2023
Copy link
Member

@sbrannen sbrannen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the changes to AbstractRoutingConnectionFactory as well.

After further consideration, the team has decided that we would like these new methods to be named initialize() instead of refresh(), since the latter implies that there is first class support for dynamic refreshes at runtime -- which is not the case.

@violetbeach
Copy link
Contributor Author

After further consideration, the team has decided that we would like these new methods to be named initialize() instead of refresh(), since the latter implies that there is first class support for dynamic refreshes at runtime -- which is not the case.

I get it.

Thank you for considering it and the kind comment!

sbrannen added a commit to sbrannen/spring-framework that referenced this pull request Sep 26, 2023
@sbrannen sbrannen closed this in 6d2d8a3 Sep 26, 2023
@sbrannen
Copy link
Member

This has been merged into main in 6d2d8a3 and revised in d50ec68.

Thanks for the contribution, and congratulations on getting your first PR for the Spring Framework merged! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: data Issues in data modules (jdbc, orm, oxm, tx) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants