JAVA-2682: Create a Quarkus Cassandra extension #1

tomekl007 · 2020-03-27T14:31:29Z

No description provided.

…gin.version` 3.0.0-M4 to start all Junit5 tests

adutra · 2020-03-29T17:42:24Z

General remark: I'm surprised by the general layout.

Why do we need to inherit from JBoss parent POM?
Why do we need a build-parent module, couldn't it be merged into the root POM?
If poms/bom is the runtime BOM, can we name it bom-runtime for clarity?
The runtime module is called cassandra-quarkus-client, shouldn't we rename it cassandra-quarkus-client-runtime for symmetry with cassandra-quarkus-client-deployment?
Why do we need the intermediary cassandra-quarkus-extensions POM?
Why is cassandra-quarkus-integration-tests a POM module with a child JAR module?
Some properties are declared and only used in one place, I don't find it particularly useful and creates a level of indirection that makes it hard to understand the POMs.

I haven't looked more in detail but it seems that the general structure of the project could be much simpler.

poms/bom/pom.xml

extensions/cassandra-client/deployment/pom.xml

tomekl007 · 2020-03-30T08:31:30Z

General remark: I'm surprised by the general layout.

Why do we need to inherit from JBoss parent POM?

Why do we need a build-parent module, couldn't it be merged into the root POM?

If poms/bom is the runtime BOM, can we name it bom-runtime for clarity?

The runtime module is called cassandra-quarkus-client, shouldn't we rename it cassandra-quarkus-client-runtime for symmetry with cassandra-quarkus-client-deployment?

Why do we need the intermediary cassandra-quarkus-extensions POM?

Why is cassandra-quarkus-integration-tests a POM module with a child JAR module?

Some properties are declared and only used in one place, I don't find it particularly useful and creates a level of indirection that makes it hard to understand the POMs.

I haven't looked more in detail but it seems that the general structure of the project could be much simpler.

Overall, I wanted to mirror the structure from:

https://github.com/riptano/quarkus
(I was also using https://github.com/Naros/debezium-quarkus and https://github.com/apache/camel-quarkus as the points of reference)

quarkus and extensions are inheriting this JBoss parent so If we want to be compatible I think we should as well.
The same situation - the quarkus project has dedicated build-parent module (and both extensions)
It seems that in the quarkus, for the runtime the suffix is omitted. I think that it is because the deployment "contains" runtime so those dependencies are not only for runtime.
It is the same situation - this is the naming pattern that I took from the quarkus repo.
I think that we should unify it with quarkus and other extensions - for all of those, there is intermediary extensions module, and inside of it there are extensions (as a separate module, in our case it is cassandra-quarkus-extensions)
Same as above.
I can inline them, I don't have a preference here.

So I totally agree that it could be simplified, but If we want to be unified with quarkus repo, and other extensions, we should follow this structure. Please let me know what do you think.

…Item` and `JniRuntimeAccessBuildItem`. Set `TIMESTAMP_GENERATOR_FORCE_JAVA_CLOCK` to `true` because jnr is not supported

… to return always false. It will not call `Native.getProcessId` and JNR

…f to Native image, instead of additionalArgs in maven

olim7t · 2020-03-30T23:22:08Z

The runtime module is called cassandra-quarkus-client, shouldn't we rename it cassandra-quarkus-client-runtime for symmetry with cassandra-quarkus-client-deployment?

Omitting the suffix for the runtime module is a Quarkus naming convention, see the extension guide.

On that note, the Quarkus maven plugin has a create-extension goal to bootstrap a new extension, @tomekl007 did you use that to initialize the project? If not, we could at least check that the structure we use is consistent with what it would have generated.

EDIT - output of the Maven plugin (but I think it's intended for a project that already has the extensions/ module):

$ mvn io.quarkus:quarkus-maven-plugin:1.3.1.Final:create-extension -N \
    -Dquarkus.artifactIdBase=cassandra-client \
    -Dquarkus.groupId=com.datastax.oss \
    -Dquarkus.nameBase="Cassandra client" \
    -Dversion=1.0.0-SNAPSHOT
$ find cassandra-client -type f
cassandra-client/runtime/pom.xml
cassandra-client/pom.xml
cassandra-client/deployment/pom.xml
cassandra-client/deployment/src/main/java/com/datastax/oss/cassandra/client/deployment/CassandraClientProcessor.java

tomekl007 · 2020-03-31T06:58:28Z

The runtime module is called cassandra-quarkus-client, shouldn't we rename it cassandra-quarkus-client-runtime for symmetry with cassandra-quarkus-client-deployment?

Omitting the suffix for the runtime module is a Quarkus naming convention, see the extension guide.

On that note, the Quarkus maven plugin has a create-extension goal to bootstrap a new extension, @tomekl007 did you use that to initialize the project? If not, we could at least check that the structure we use is consistent with what it would have generated.

EDIT - output of the Maven plugin (but I think it's intended for a project that already has the extensions/ module):
$ mvn io.quarkus:quarkus-maven-plugin:1.3.1.Final:create-extension -N \
    -Dquarkus.artifactIdBase=cassandra-client \
    -Dquarkus.groupId=com.datastax.oss \
    -Dquarkus.nameBase="Cassandra client" \
    -Dversion=1.0.0-SNAPSHOT
$ find cassandra-client -type f
cassandra-client/runtime/pom.xml
cassandra-client/pom.xml
cassandra-client/deployment/pom.xml
cassandra-client/deployment/src/main/java/com/datastax/oss/cassandra/client/deployment/CassandraClientProcessor.java

Yes, I was using that, but exactly what you wrote " I think it's intended for a project that already has the extensions/ module", also this is intended only for extensions that are in the quarkus repo and rely on the structure that it's in that repo, so we cannot use it for our case.

…s-extension

…orted in the BOM of this project

into java2682

adutra · 2020-04-02T12:46:17Z

To reviewers: I pushed a few commits:

Project structure was simplified a bit more, you may need to reimport the project in your IDE, my apologies for the trouble.
Integration tests now run in the right Maven phase.
If you want to run the native tests, do mvn clean verify -Dnative. Make sure you have GRAALVM_HOME set.

runtime/src/main/java/com/datastax/oss/quarkus/runtime/AbstractCassandraClientProducer.java

absurdfarce · 2020-04-02T19:34:39Z

runtime/src/main/java/com/datastax/oss/quarkus/runtime/graal/NativeSubstitutions.java

+import com.oracle.svm.core.annotate.TargetClass;
+
+@TargetClass(Native.class)
+final class NativeSubstitutions {


I'm musing a bit here... do we want to include isCurrentTimeMicrosAvailable() here as well (with a constant return value of false) until we get the native stuff sorted out? We're side-stepping the issue entirely now by forcing the use of a Java clock but my thought was that that's an indirect consequence of not having native support.

On the other hand that clock is the only consumer of this functionality... so I can see a counter-argument which says adding it here is just one more thing we have to unset/change when we want to confirm native functionality is fine.

I guess I could go either way here.

We will improve it - see my comments here:
https://datastax-oss.atlassian.net/browse/JAVA-2696
I think that Substitutions should be used only if there is no other way to tackle the problem.
For the isCurrentTimeMicrosAvailable we have force_java_clock setting so it should be ok to use it.

deployment/src/test/java/com/datastax/oss/quarkus/CassandraClientConfigTest.java

...ion-tests/src/main/java/com/datastax/oss/quarkus/dao/nameconverters/NameConverterEntity.java

absurdfarce

While working on other things I wasn't closely following the earlier conversations around simplifying the overall structure. But what we have now looks entirely reasonable to me; it seems like a robust skeleton on which we can base future work. All of my comments reflect future work; I don't see anything which prevents us from merging this PR now, especially since this is pretty foundational code.

tomekl007 · 2020-04-03T07:46:09Z

All pending comments were addressed as separate JIRA tickets, see:
https://datastax-oss.atlassian.net/browse/JAVA-2681

* Set version to 1.2.0-alpha1-SNAPSHOT * Upgrade to Quarkus 3.0.0.Alpha4 * Bulk replace javax.* with jakarta.* * Update Mutiny code for Quarkus 3 * Add missing Override annotations * Update to Quarkus 3.0.0.Beta1 (#1) Co-authored-by: Madhavan Sridharan <madhavan.sridharan@datastax.com> --------- Co-authored-by: Madhavan <msmygit@users.noreply.github.com> Co-authored-by: Madhavan Sridharan <madhavan.sridharan@datastax.com>

tomekl007 added 9 commits March 27, 2020 11:42

skeleton of cassandra extension maven project

e15cbdb

JAVA-2709 maven project is building succesfullly

aa78dc8

JAVA-2709 remove leading new line

4b435f2

JAVA-2709 port code of cassandra-extension

4e6e837

JAVA-2709 change package names to artifactId, use `maven-surefire-plu…

ba3a203

…gin.version` 3.0.0-M4 to start all Junit5 tests

JAVA-2709 add mvn format plugin and reformat all sources

2c16e02

add gitignore

44bb968

JAVA-2709 add integration-test module

abf3e85

JAVA-2709 add changelog entry

b4787e7

tomekl007 requested review from adutra, absurdfarce and olim7t March 27, 2020 14:32

adutra mentioned this pull request Mar 29, 2020

JAVA-2682: Create a Quarkus Cassandra extension riptano/quarkus#1

Closed

adutra reviewed Mar 29, 2020

View reviewed changes

poms/bom/pom.xml Outdated Show resolved Hide resolved

adutra reviewed Mar 29, 2020

View reviewed changes

poms/bom/pom.xml Outdated Show resolved Hide resolved

adutra reviewed Mar 29, 2020

View reviewed changes

extensions/cassandra-client/deployment/pom.xml Outdated Show resolved Hide resolved

JAVA-2709 add xml format plugin and reformat all POMs

20f781a

tomekl007 mentioned this pull request Mar 30, 2020

Cassandra DataStax Extension quarkusio/quarkus#7876

Closed

tomekl007 added 5 commits March 30, 2020 13:46

JAVA-2696 register for reflection and JNI using `ReflectiveClassBuild…

1af4f55

…Item` and `JniRuntimeAccessBuildItem`. Set `TIMESTAMP_GENERATOR_FORCE_JAVA_CLOCK` to `true` because jnr is not supported

JAVA-2696 remove import of ASM

8a23099

JAVA-2696 use substitution to change method isGetProcessIdAvailable…

e04bfda

… to return always false. It will not call `Native.getProcessId` and JNR

JAVA-2696 use NativeImageResourceBuildItem for adding reference.con…

c3ff6b2

…f to Native image, instead of additionalArgs in maven

JAVA-2696 remove JNI import

122f22c

tomekl007 added 3 commits March 31, 2020 10:01

JAVA-2709 flatten extensions module to have only one cassandra-quarku…

f568cbe

…s-extension

JAVA-2709 flatten integration-tests module

3c5a9d8

JAVA-2709 inherit quarkus-parent instead of jboss-parent

0418261

tomekl007 added 4 commits April 2, 2020 11:11

JAVA-2709 get rid of inheriting quarkus-parent BOM, because it is imp…

ceb1c74

…orted in the BOM of this project

JAVA-2696 add docs for MetadataManager RuntimeInitialization

495091d

JAVA-2696 add all driver classes that can be used via reflection

cd2bf43

Merge branch 'java2682' of https://github.com/datastax/cassandra-quarkus

d891dcf

into java2682

tomekl007 requested review from adutra and olim7t April 2, 2020 09:31

adutra added 6 commits April 2, 2020 12:49

Simplify structure even further

4dbd554

Use failsafe to run integration tests

6534d7b

Include logback in annotation processor classpath

d9644ee

Include driver-core as direct dependency

1787f67

Add docs on ITs

ba5b487

Use JBoss LogManager in integration tests

1e3cfe9

adutra approved these changes Apr 2, 2020

View reviewed changes