-
Notifications
You must be signed in to change notification settings - Fork 602
Hazelcast integration with Hikari connection pool #694
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
Hazelcast integration with Hikari connection pool #694
Conversation
| <dependency> | ||
| <groupId>com.h2database</groupId> | ||
| <artifactId>h2</artifactId> | ||
| <version>2.2.224</version> | ||
| <scope>runtime</scope> | ||
| </dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The instructions say to package this jar and add it to lib dir.
You probably don't want to add h2 there, should this be in test scope?
| - <h3>hikari-connection-pool</h3> | ||
| A sample demo code integrate Hazelcast with Hikari connection pool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We provide an integration with Hikari via JdbcDataConnection.
https://docs.hazelcast.com/hazelcast/5.5/data-connections/data-connections-configuration#JDBC
It provides a nice integration with the Hazelcast configuration and the resources cleanup is tied to the data connection (either dynamically when used from SQL, or bound to cluster lifecycle when configured in XML).
Would you like to rework the sample with the JdbcDataConnection instead?
| <plugin> | ||
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-compiler-plugin</artifactId> | ||
| <version>3.13.0</version> | ||
| <configuration> | ||
| <source>${java.version}</source> | ||
| <target>${java.version}</target> | ||
| </configuration> | ||
| </plugin> | ||
| <plugin> | ||
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-resources-plugin</artifactId> | ||
| <version>3.3.1</version> | ||
| </plugin> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these necessary? Where is the java.version defined?
| import java.sql.DriverManager; | ||
| import java.sql.SQLException; | ||
|
|
||
| public class JDBCBasicConnection { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used anywhere?
| import java.sql.SQLException; | ||
|
|
||
| public class HikariDataSourcePool { | ||
| private static HikariDataSource hikariDataSource = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a static in this way makes it difficult to test. We don't want to give our users wrong advice.
This should be tied to the life of a MapStore instance.
| throw new RuntimeException("Ops! Hikari datasource not available."); | ||
| } | ||
| } catch (SQLException e) { | ||
| throw new RuntimeException("Exception while creating database connection." + e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provide the e as cause to the RuntimeException to get full stacktrace.
(same for all catch blocks in the PR).
|
|
||
| import static org.junit.jupiter.api.Assertions.*; | ||
|
|
||
| class EmployeeMapStoreTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you plan to implement this test?
| @Getter | ||
| @Setter | ||
| @Builder | ||
| public class Employee { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just use record and get rid of the Lombok dependency.
| map.forEach((identity, employee) -> { | ||
|
|
||
| try { | ||
| preparedStatement.setInt(1, employee.getEmpId()); | ||
| preparedStatement.setString(2, employee.getName()); | ||
| preparedStatement.setDouble(3, employee.getSalary()); | ||
| } catch (SQLException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this missing preparedStatement.addBatch() call?
| @Override | ||
| public void delete(Integer integer) { | ||
| } | ||
|
|
||
| @Override | ||
| public void deleteAll(Collection<Integer> collection) { | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implement these as well. Alternatively implementing the MapLoader interface only would be enough for the demonstration.
yuce
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
Just a few ideas...
| //map.forEach(this::store); | ||
| //or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this leftover?
|
|
||
| // Handle the results if needed | ||
| for (int result : batchResults) { | ||
| //TODO: add this if needed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to print the results?
| private static HikariDataSource hikariDataSource = null; | ||
|
|
||
| public HikariDataSourcePool() { | ||
| if (null != hikariDataSource) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HikariDataSourcePool is not thread-safe due to hikariDataSource. It would be ideal to make it thread-safe (e.g., using double checked locking), or mark it thread unsafe.
|
@rajangare Latest changes don't make this class thread-safe, but it would be good if you could add a comment that mentions this issue. |
@yuce added the comments on top, for thread-safe. do you have any suggestion to make it thread-safe please help here. |
yuce
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks.
hikariDataSource field should be synchronized, using something like double checked locking:
https://www.baeldung.com/java-singleton-double-checked-locking
But you've added a comment about the class is being thread-unsafe.
I think that's OK for now.
Hazelcast Integration with Hikari Connection Pool
To create Hikari connection pool, first create database table mapping with Hazelcast in-memory map.
In this demo, I have used EMPLOYEE table created on H2 DB, which is mapped with employee-map.
Create Hikari data source from HikariConfig, with database credentials and connection pool configuration e.g. pool size, connection timeout etc.
Get the connection from Hikari Data Source.
Implement the Map Store Factory from MapLoader<K, V>, MapStore<K, V> and override all the necessary methods.
Register the MapStore into hazelcast.xml config.
... com.demo.maps.EmployeeMapStore ...Database connection information can be externalized from hazelcast.xml. Build the above classes in a jar and copy them inside Hazelcast user-lib directory under the Hazelcast installation.