petrov-mg commented on code in PR #360:
URL: https://github.com/apache/ignite-extensions/pull/360#discussion_r3582186808
##########
modules/hibernate-ext/hibernate/src/test/java/org/apache/ignite/cache/store/hibernate/CacheHibernateStoreFactorySelfTest.java:
##########
@@ -274,23 +252,28 @@ public static class DummySessionFactory implements
SessionFactory {
return null;
}
+ /** {@inheritDoc} */
+ @Override public Set<String> getDefinedFetchProfileNames() {
+ return Set.of();
Review Comment:
Does `null` make sense here? If not, it may be better to use
`Collections.emptySet()`
##########
modules/hibernate-ext/hibernate/src/test/java/org/apache/ignite/cache/hibernate/HibernateL2CacheTransactionalSelfTest.java:
##########
@@ -85,10 +86,7 @@ public static class TestTmFactory implements
Factory<TransactionManager> {
@Override protected void afterTestsStopped() throws Exception {
super.afterTestsStopped();
- if (jotm != null)
- jotm.stop();
-
- jotm = null;
+ txMgr = null;
Review Comment:
Do we need to clear `userTx`?
##########
modules/hibernate-ext/hibernate/src/test/java/org/apache/ignite/cache/hibernate/HibernateL2CacheSelfTest.java:
##########
@@ -1764,13 +1767,21 @@ private void testRegionClear(AccessType accessType)
throws Exception {
* @param absentNames Absent entities' names.
*/
private void assertNaturalIdCache(SessionFactory sesFactory, Map<String,
Integer> nameToId, String... absentNames) {
- sesFactory.getStatistics().clear();
-
- CacheRegionStatistics stats =
sesFactory.getStatistics().getCacheRegionStatistics(NATURAL_ID_REGION);
+ // H6: Natural ID cache doesn't auto-invalidate on mutation across
SessionFactories,
Review Comment:
I’m certainly no Hibernate guru, but why don’t we just replace
```
sesFactory.getStatistics().clear();
CacheRegionStatistics stats =
sesFactory.getStatistics().getCacheRegionStatistics(NATURAL_ID_REGION);
```
with
```
Statistics statistics = sesFactory.getStatistics();
statistics.clear();
CacheRegionStatistics stats =
statistics.getDomainDataRegionStatistics(NATURAL_ID_REGION);
```
It seems the tests are ok.
##########
modules/hibernate-ext/hibernate/src/test/java/org/apache/ignite/cache/hibernate/HibernateL2CacheConfigurationSelfTest.java:
##########
@@ -154,7 +154,7 @@ protected Configuration hibernateConfiguration(String
igniteInstanceName) {
cfg.setProperty(CACHE_REGION_FACTORY,
HibernateRegionFactory.class.getName());
- cfg.setProperty(RELEASE_CONNECTIONS, "on_close");
+ cfg.setProperty(CONNECTION_HANDLING, "DELAYED_ACQUISITION_AND_HOLD");
Review Comment:
It is better to use `DELAYED_ACQUISITION_AND_HOLD.name()`
##########
modules/hibernate-ext/hibernate/src/test/java/org/apache/ignite/cache/store/hibernate/CacheHibernateBlobStoreSelfTest.java:
##########
@@ -50,7 +50,7 @@ public CacheHibernateBlobStoreSelfTest() throws Exception {
try {
s.createQuery("delete from " +
CacheHibernateBlobStoreEntry.class.getSimpleName())
- .setFlushMode(FlushMode.ALWAYS).executeUpdate();
+ .setFlushMode(FlushModeType.AUTO).executeUpdate();
Review Comment:
I think it is better to keep the existing logic unchanged and use
`.setHibernateFlushMode(FlushMode.ALWAYS)`.
##########
modules/hibernate-ext/hibernate/pom.xml:
##########
@@ -44,42 +44,48 @@
</dependency>
<dependency>
- <groupId>org.hibernate</groupId>
+ <groupId>org.hibernate.orm</groupId>
<artifactId>hibernate-core</artifactId>
<version>${hibernate.core.version}</version>
</dependency>
+ <dependency>
+ <groupId>jakarta.persistence</groupId>
+ <artifactId>jakarta.persistence-api</artifactId>
+ <version>3.1.0</version>
Review Comment:
Versions need to be declared via maven properties. And we should
double-check whether new dependencies are really required.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
