quarkiverse / quarkus-jnosql

The Quarkus JNoSql Extension adds support for JNoSQL, an implementation of Jakarta NoSQL.
http://www.jnosql.org/
Apache License 2.0
13 stars 4 forks source link

Upgrade jnosql.version to 1.0.2 #59

Closed dearrudam closed 1 year ago

dearrudam commented 1 year ago

Changes:

dearrudam commented 1 year ago

@amoscatelli , with this upgrade to JNoSQL 1.0.2 the codebase will be ready to come back to think about the #45 opportunity. Please, could you review this PR?

amoscatelli commented 1 year ago

@dearrudam it seems there is an error : No implementation of ClassConverter found via ServiceLoader

I can't accept this if this doesn't work :/

dearrudam commented 1 year ago

@amoscatelli there's no problem! Sorry for opening this PR before to test it properly ... I just performed mvn verify but I forgot to run it using the native profile...I'm still working on it.

Let me try to explain my scenario by now: The new JNoSQL version uses the ServiceLoader feature to discover the services, but the native-compilation requires the use of the flag -H:+UseServiceLoaderFeature to enable the auto-discovery and registry of the services from each META-INF/services/* file on each dependency. According to Quarkus documentation (https://quarkus.io/guides/building-native-image#quarkus-native-pkg-native-config_quarkus.native.auto-service-loader-registration) it's needed to add a property to enable this flag:

<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">
 <!-- skip -->
 <properties>
   <!-- skip other properties -->
   <quarkus.native.auto-service-loader-registration>true</quarkus.native.auto-service-loader-registration>
   <!-- skip other properties -->
 </properties>
 <!-- skip -->
</project>

But it looks like the quarkus-jnosql-document-elasticsearch-integration-tests is not compiling in native one as expected. Here's the log info: https://github.com/quarkiverse/quarkus-jnosql/actions/runs/6474075432/job/17578197542?pr=59#step:6:2195

Well, I'm still working on it. Feel free to take a look at my branch. I'm open to suggestions and support, of course!

amoscatelli commented 1 year ago

@dearrudam Yea I understand perfectly. I don't think you can use quarkus.native.auto-service-loader-registration as a maven property.

You have to use it in the application.properties during the tests. Give it a try, I bet it'll work.

amoscatelli commented 1 year ago

@dearrudam I also digged more into this ... Please use this : https://quarkus.io/guides/writing-extensions#service-files

amoscatelli commented 1 year ago

@dearrudam do you need help ?

amoscatelli commented 1 year ago

@dearrudam it seems much better now yet we have some problems with Hezelcast we didn't before

What's changed about the Hezelcast JNoSQL driver lately ?

I see this error about com.sun.jmx.mbeanserver.JmxMBeanServer :

Error: java.util.concurrent.ExecutionException: com.oracle.graal.pointsto.constraints.UnsupportedFeatureException: Detected a MBean server in the image heap. This is currently not supported, but could be changed in the future. Management beans are registered in many global caches that would need to be cleared and properly re-built at image build time. Class of disallowed object: com.sun.jmx.mbeanserver.JmxMBeanServer  To see how this object got instantiated use --trace-object-instantiation=com.sun.jmx.mbeanserver.JmxMBeanServer. The object was probably created by a class initializer and is reachable from a static field. You can request class initialization at image runtime by using the option --initialize-at-run-time=<class-name>. Or you can write your own initialization methods and call them explicitly from your main entry point.

Maybe the Hazelcast native driver version were updated ?

amoscatelli commented 1 year ago

It seems not, the version between 1.0.1 and 1.0.2 didn't change :

        <dependency>
            <groupId>com.hazelcast</groupId>
            <artifactId>hazelcast</artifactId>
            <version>5.3.1</version>
        </dependency>
otaviojava commented 1 year ago

I will update Hazelcast and deploy it as SNAPSHOT. Could you try this 1.0.3-SNAPSHOT?

https://ci.eclipse.org/jnosql/view/Deploy/job/jnosql-databases-deploy/151/console

dearrudam commented 1 year ago

As @otaviojava asked, I'll try to build it using the 103-SNAPSHOT version...

amoscatelli commented 1 year ago

I will update Hazelcast and deploy it as SNAPSHOT. Could you try this 1.0.3-SNAPSHOT?

https://ci.eclipse.org/jnosql/view/Deploy/job/jnosql-databases-deploy/151/console

why are you suggesting this ? is this just an attempt or do you have any specific suspect on what's the problem ?

amoscatelli commented 1 year ago

It seems not, the version between 1.0.1 and 1.0.2 didn't change :

        <dependency>
            <groupId>com.hazelcast</groupId>
            <artifactId>hazelcast</artifactId>
            <version>5.3.1</version>
        </dependency>

What I meant here is that since the driver didn't change, I can't understand why this is not working ... I can't find anything suspicious about the pull request changed code ....

dearrudam commented 1 year ago

@otaviojava and @amoscatelli , I think that I solved the native compilation issues... please, review this PR?

dearrudam commented 1 year ago

It seems not, the version between 1.0.1 and 1.0.2 didn't change :

        <dependency>
            <groupId>com.hazelcast</groupId>
            <artifactId>hazelcast</artifactId>
            <version>5.3.1</version>
        </dependency>

What I meant here is that since the driver didn't change, I can't understand why this is not working ... I can't find anything suspicious about the pull request changed code ....

Actually, the drive is provided by the com.hazelcast:quarkus-hazelcast-client dependency.

dearrudam commented 1 year ago

Thanks, @amoscatelli, for informing me about the BuildRuntimeInitializedClassBuildItem class utilization.

I've just moved the only necessary classes that should be postponed to the runtime phase in the native compilation.

@amoscatelli and @otaviojava, could you review this PR again?

amoscatelli commented 1 year ago

@dearrudam LGTM thx for your work and patience, this is a very fine PR now I also removed a couple of empty files, as soon as the pipeline ends I am going to merge