search

datastax / cassandra-quarkus

An Apache Cassandra(R) extension for Quarkus
Apache License 2.0
40 stars 28 forks source link

JAVA-2682: Create a Quarkus Cassandra extension #1

Closed tomekl007 closed 4 years ago

adutra commented 4 years ago

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

  1. Why do we need to inherit from JBoss parent POM?
  2. Why do we need a build-parent module, couldn't it be merged into the root POM?
  3. If poms/bom is the runtime BOM, can we name it bom-runtime for clarity?
  4. The runtime module is called cassandra-quarkus-client, shouldn't we rename it cassandra-quarkus-client-runtime for symmetry with cassandra-quarkus-client-deployment?
  5. Why do we need the intermediary cassandra-quarkus-extensions POM?
  6. Why is cassandra-quarkus-integration-tests a POM module with a child JAR module?
  7. 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.

tomekl007 commented 4 years ago

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

  1. Why do we need to inherit from JBoss parent POM?
  2. Why do we need a build-parent module, couldn't it be merged into the root POM?
  3. If poms/bom is the runtime BOM, can we name it bom-runtime for clarity?
  4. The runtime module is called cassandra-quarkus-client, shouldn't we rename it cassandra-quarkus-client-runtime for symmetry with cassandra-quarkus-client-deployment?
  5. Why do we need the intermediary cassandra-quarkus-extensions POM?
  6. Why is cassandra-quarkus-integration-tests a POM module with a child JAR module?
  7. 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:

  1. quarkus and extensions are inheriting this JBoss parent so If we want to be compatible I think we should as well.
  2. The same situation - the quarkus project has dedicated build-parent module (and both extensions)
  3. 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.
  4. It is the same situation - this is the naming pattern that I took from the quarkus repo.
  5. 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)
  6. Same as above.
  7. 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.

olim7t commented 4 years ago

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 commented 4 years ago

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.

tomekl007 commented 4 years ago

@adutra

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

  1. Why do we need to inherit from JBoss parent POM?
  2. Why do we need a build-parent module, couldn't it be merged into the root POM?
  3. If poms/bom is the runtime BOM, can we name it bom-runtime for clarity?
  4. The runtime module is called cassandra-quarkus-client, shouldn't we rename it cassandra-quarkus-client-runtime for symmetry with cassandra-quarkus-client-deployment?
  5. Why do we need the intermediary cassandra-quarkus-extensions POM?
  6. Why is cassandra-quarkus-integration-tests a POM module with a child JAR module?
  7. 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.

I've simplified the project structure:

  1. I change it to inherit <artifactId>quarkus-parent</artifactId> because this is recommended on the https://quarkus.io/guides/writing-extensions#maven-setup
  2. I've removed build-parent and merged it with the root POM.
  3. Naming convention https://quarkus.io/guides/writing-extensions#maven-setup
  4. Naming convention
  5. I've flattened it
  6. I've flattened it.
  7. I've inlined properties that are used only one time.
adutra commented 4 years ago

To reviewers: I pushed a few commits:

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

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