You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by "ruchiD (via GitHub)" <gi...@apache.org> on 2023/04/13 14:54:27 UTC
[GitHub] [fineract] ruchiD opened a new pull request, #3120: FINERACT-1913-Initializing-db-connection-pool-at-startup
ruchiD opened a new pull request, #3120:
URL: https://github.com/apache/fineract/pull/3120
## Description
Changes to initialize database connection pool at startup for tenants.
Describe the changes made and why they were made.
Ignore if these details are present on the associated [Apache Fineract JIRA ticket](https://github.com/apache/fineract/pull/1284).
## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
- [ ] Write the commit message as per https://github.com/apache/fineract/#pull-requests
- [ ] Acknowledge that we will not review PRs that are not passing the build _("green")_ - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
- [ ] Create/update unit or integration tests for verifying the changes made.
- [ ] Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
- [ ] Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
- [ ] Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)
FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ruchiD commented on a diff in pull request #3120: FINERACT-1913-Initializing-db-connection-pool-at-startup
Posted by "ruchiD (via GitHub)" <gi...@apache.org>.
ruchiD commented on code in PR #3120:
URL: https://github.com/apache/fineract/pull/3120#discussion_r1166269500
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/service/database/TomcatJdbcDataSourcePerTenantService.java:
##########
@@ -73,4 +83,29 @@ public DataSource retrieveDataSource() {
return actualDataSource;
}
+
+ @Override
+ public void onApplicationEvent(ContextRefreshedEvent event) {
+ final List<FineractPlatformTenant> allTenants = tenantDetailsService.findAllTenants();
+ for (final FineractPlatformTenant tenant : allTenants) {
+ initializeDataSourceConnection(tenant);
+ }
+ }
+
+ private void initializeDataSourceConnection(FineractPlatformTenant tenant) {
+ log.debug("Initializing database connection for {}", tenant.getName());
+ final FineractPlatformTenantConnection tenantConnection = tenant.getConnection();
+ if (!TENANT_TO_DATA_SOURCE_MAP.containsKey(tenantConnection.getConnectionId())) {
+ DataSource tenantSpecificDataSource = dataSourcePerTenantServiceFactory.createNewDataSourceFor(tenantConnection);
+ try (Connection connection = tenantSpecificDataSource.getConnection()) {
+ String url = connection.getMetaData().getURL();
+ log.debug("Established database connection with URL {}", url);
+ } catch (SQLException e) {
+ log.error("Error while initializing database connection for {}", tenant.getName(), e.getMessage());
Review Comment:
Fixed
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/service/database/TomcatJdbcDataSourcePerTenantService.java:
##########
@@ -73,4 +83,29 @@ public DataSource retrieveDataSource() {
return actualDataSource;
}
+
+ @Override
+ public void onApplicationEvent(ContextRefreshedEvent event) {
+ final List<FineractPlatformTenant> allTenants = tenantDetailsService.findAllTenants();
+ for (final FineractPlatformTenant tenant : allTenants) {
+ initializeDataSourceConnection(tenant);
+ }
+ }
+
+ private void initializeDataSourceConnection(FineractPlatformTenant tenant) {
+ log.debug("Initializing database connection for {}", tenant.getName());
+ final FineractPlatformTenantConnection tenantConnection = tenant.getConnection();
+ if (!TENANT_TO_DATA_SOURCE_MAP.containsKey(tenantConnection.getConnectionId())) {
Review Comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] adamsaghy commented on pull request #3120: FINERACT-1913-Initializing-db-connection-pool-at-startup
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #3120:
URL: https://github.com/apache/fineract/pull/3120#issuecomment-1507509207
@ruchiD it is failing on spotbugs check
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] adamsaghy commented on a diff in pull request #3120: FINERACT-1913-Initializing-db-connection-pool-at-startup
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #3120:
URL: https://github.com/apache/fineract/pull/3120#discussion_r1165671279
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/service/database/TomcatJdbcDataSourcePerTenantService.java:
##########
@@ -73,4 +83,29 @@ public DataSource retrieveDataSource() {
return actualDataSource;
}
+
+ @Override
+ public void onApplicationEvent(ContextRefreshedEvent event) {
+ final List<FineractPlatformTenant> allTenants = tenantDetailsService.findAllTenants();
+ for (final FineractPlatformTenant tenant : allTenants) {
+ initializeDataSourceConnection(tenant);
+ }
+ }
+
+ private void initializeDataSourceConnection(FineractPlatformTenant tenant) {
+ log.debug("Initializing database connection for {}", tenant.getName());
+ final FineractPlatformTenantConnection tenantConnection = tenant.getConnection();
+ if (!TENANT_TO_DATA_SOURCE_MAP.containsKey(tenantConnection.getConnectionId())) {
+ DataSource tenantSpecificDataSource = dataSourcePerTenantServiceFactory.createNewDataSourceFor(tenantConnection);
+ try (Connection connection = tenantSpecificDataSource.getConnection()) {
+ String url = connection.getMetaData().getURL();
+ log.debug("Established database connection with URL {}", url);
+ } catch (SQLException e) {
+ log.error("Error while initializing database connection for {}", tenant.getName(), e.getMessage());
Review Comment:
Do we need to catch it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] galovics commented on a diff in pull request #3120: FINERACT-1913-Initializing-db-connection-pool-at-startup
Posted by "galovics (via GitHub)" <gi...@apache.org>.
galovics commented on code in PR #3120:
URL: https://github.com/apache/fineract/pull/3120#discussion_r1165802413
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/service/database/TomcatJdbcDataSourcePerTenantService.java:
##########
@@ -73,4 +83,29 @@ public DataSource retrieveDataSource() {
return actualDataSource;
}
+
+ @Override
+ public void onApplicationEvent(ContextRefreshedEvent event) {
+ final List<FineractPlatformTenant> allTenants = tenantDetailsService.findAllTenants();
+ for (final FineractPlatformTenant tenant : allTenants) {
+ initializeDataSourceConnection(tenant);
+ }
+ }
+
+ private void initializeDataSourceConnection(FineractPlatformTenant tenant) {
+ log.debug("Initializing database connection for {}", tenant.getName());
+ final FineractPlatformTenantConnection tenantConnection = tenant.getConnection();
+ if (!TENANT_TO_DATA_SOURCE_MAP.containsKey(tenantConnection.getConnectionId())) {
Review Comment:
This is not thread-safe. The application events by default are processed synchronously by the event listeners but Fineract has a different strategy configured, i.e. it's using a custom task executor to execute the event listeners (see SpringConfig class in Fineract, applicationEventMulticaster).
Therefore the map operations are needed to be made thread-safe.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/service/database/TomcatJdbcDataSourcePerTenantService.java:
##########
@@ -73,4 +83,29 @@ public DataSource retrieveDataSource() {
return actualDataSource;
}
+
+ @Override
+ public void onApplicationEvent(ContextRefreshedEvent event) {
+ final List<FineractPlatformTenant> allTenants = tenantDetailsService.findAllTenants();
+ for (final FineractPlatformTenant tenant : allTenants) {
+ initializeDataSourceConnection(tenant);
+ }
+ }
+
+ private void initializeDataSourceConnection(FineractPlatformTenant tenant) {
+ log.debug("Initializing database connection for {}", tenant.getName());
+ final FineractPlatformTenantConnection tenantConnection = tenant.getConnection();
+ if (!TENANT_TO_DATA_SOURCE_MAP.containsKey(tenantConnection.getConnectionId())) {
+ DataSource tenantSpecificDataSource = dataSourcePerTenantServiceFactory.createNewDataSourceFor(tenantConnection);
+ try (Connection connection = tenantSpecificDataSource.getConnection()) {
+ String url = connection.getMetaData().getURL();
+ log.debug("Established database connection with URL {}", url);
+ } catch (SQLException e) {
+ log.error("Error while initializing database connection for {}", tenant.getName(), e.getMessage());
Review Comment:
I think it's reasonable to catch it since the initialization process failing is not fatal to the application itself. One thing I'd do however is to pass the exception itself in the log message, not the exception message.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/service/database/TomcatJdbcDataSourcePerTenantService.java:
##########
@@ -73,4 +83,29 @@ public DataSource retrieveDataSource() {
return actualDataSource;
}
+
+ @Override
+ public void onApplicationEvent(ContextRefreshedEvent event) {
+ final List<FineractPlatformTenant> allTenants = tenantDetailsService.findAllTenants();
+ for (final FineractPlatformTenant tenant : allTenants) {
+ initializeDataSourceConnection(tenant);
+ }
+ }
+
+ private void initializeDataSourceConnection(FineractPlatformTenant tenant) {
+ log.debug("Initializing database connection for {}", tenant.getName());
+ final FineractPlatformTenantConnection tenantConnection = tenant.getConnection();
+ if (!TENANT_TO_DATA_SOURCE_MAP.containsKey(tenantConnection.getConnectionId())) {
+ DataSource tenantSpecificDataSource = dataSourcePerTenantServiceFactory.createNewDataSourceFor(tenantConnection);
+ try (Connection connection = tenantSpecificDataSource.getConnection()) {
+ String url = connection.getMetaData().getURL();
+ log.debug("Established database connection with URL {}", url);
+ } catch (SQLException e) {
+ log.error("Error while initializing database connection for {}", tenant.getName(), e.getMessage());
+ }
+ TENANT_TO_DATA_SOURCE_MAP.put(tenantConnection.getConnectionId(), tenantSpecificDataSource);
Review Comment:
As I wrote above, not thread-safe.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] galovics merged pull request #3120: FINERACT-1913-Initializing-db-connection-pool-at-startup
Posted by "galovics (via GitHub)" <gi...@apache.org>.
galovics merged PR #3120:
URL: https://github.com/apache/fineract/pull/3120
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ruchiD commented on a diff in pull request #3120: FINERACT-1913-Initializing-db-connection-pool-at-startup
Posted by "ruchiD (via GitHub)" <gi...@apache.org>.
ruchiD commented on code in PR #3120:
URL: https://github.com/apache/fineract/pull/3120#discussion_r1166269580
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/service/database/TomcatJdbcDataSourcePerTenantService.java:
##########
@@ -73,4 +83,29 @@ public DataSource retrieveDataSource() {
return actualDataSource;
}
+
+ @Override
+ public void onApplicationEvent(ContextRefreshedEvent event) {
+ final List<FineractPlatformTenant> allTenants = tenantDetailsService.findAllTenants();
+ for (final FineractPlatformTenant tenant : allTenants) {
+ initializeDataSourceConnection(tenant);
+ }
+ }
+
+ private void initializeDataSourceConnection(FineractPlatformTenant tenant) {
+ log.debug("Initializing database connection for {}", tenant.getName());
+ final FineractPlatformTenantConnection tenantConnection = tenant.getConnection();
+ if (!TENANT_TO_DATA_SOURCE_MAP.containsKey(tenantConnection.getConnectionId())) {
+ DataSource tenantSpecificDataSource = dataSourcePerTenantServiceFactory.createNewDataSourceFor(tenantConnection);
+ try (Connection connection = tenantSpecificDataSource.getConnection()) {
+ String url = connection.getMetaData().getURL();
+ log.debug("Established database connection with URL {}", url);
+ } catch (SQLException e) {
+ log.error("Error while initializing database connection for {}", tenant.getName(), e.getMessage());
+ }
+ TENANT_TO_DATA_SOURCE_MAP.put(tenantConnection.getConnectionId(), tenantSpecificDataSource);
Review Comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] galovics commented on a diff in pull request #3120: FINERACT-1913-Initializing-db-connection-pool-at-startup
Posted by "galovics (via GitHub)" <gi...@apache.org>.
galovics commented on code in PR #3120:
URL: https://github.com/apache/fineract/pull/3120#discussion_r1166402604
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/service/database/TomcatJdbcDataSourcePerTenantService.java:
##########
@@ -73,4 +83,31 @@ public DataSource retrieveDataSource() {
return actualDataSource;
}
+
+ @Override
+ public void onApplicationEvent(ContextRefreshedEvent event) {
+ final List<FineractPlatformTenant> allTenants = tenantDetailsService.findAllTenants();
+ for (final FineractPlatformTenant tenant : allTenants) {
+ initializeDataSourceConnection(tenant);
+ }
+ }
+
+ private void initializeDataSourceConnection(FineractPlatformTenant tenant) {
+ log.debug("Initializing database connection for {}", tenant.getName());
+ final FineractPlatformTenantConnection tenantConnection = tenant.getConnection();
+ synchronized (TENANT_TO_DATA_SOURCE_MAP) {
Review Comment:
@ruchiD this is still not thread-safe since the other accesses are not synchronized.
Let me give you a hint how this should be implemented.
First change the map to be a ConcurrentHashMap
```
private static final Map<Long, DataSource> TENANT_TO_DATA_SOURCE_MAP = new ConcurrentHashMap<>();
```
Then let's use an atomic operation on the HashMap to prevent the race conditions happening due to the check-then-act approach (where one thread is seeing a stale value and the other is not).
```
String key = tenantConnection.getConnectionId();
TENANT_TO_DATE_SOURCE_MAP.computeIfAbsent(key, (key) -> {
DataSource tenantSpecificDataSource = dataSourcePerTenantServiceFactory.createNewDataSourceFor(tenantConnection);
try (Connection connection = tenantSpecificDataSource.getConnection()) {
String url = connection.getMetaData().getURL();
log.debug("Established database connection with URL {}", url);
return tenantSpecificDataSource;
} catch (SQLException e) {
log.error("Error while initializing database connection for {}", tenant.getName(), e);
}
});
```
And similarly, you have to do the same at the other points where the map is accessed. The key thing is to use operations where ConcurrentHashMap provides atomicity guarantess (computeIfAbsent/Present/putIfAbsent/Present/etc)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org