Convert GraphQL v2 to Quarkus #1980

olim7t · 2022-07-15T17:09:22Z

Main changes:

GraphqlResourceBase and subclasses: use injection to pass the bridge and GraphqlCache, adapt multipart handling code for Quarkus.
GraphqlCache: use Quarkus config for enableDefaultKeyspace

Everything else is pretty much a copy-paste of the existing sgv2-graphqlapi, with a few changes to the imports. Note that fetcher implementations still rely on the synchronous methods of StargateBridgeClient; with the async/reactive bridge it would now be possible to make all fetchers async, but I consider this beyond the scope of this PR, see #421.

TODO:

find a way to add stargate.graphql.enable-playground to the config (how do we condition the inclusion of a REST resource at runtime?)
adapt (move?) the integration tests in testing
remove the original sgv2-graphqlapi at the root of the repo
handle OVERLOADED error (see GraphqlResourceBase.executeAsync)

apis/sgv2-graphqlapi/src/main/java/io/stargate/sgv2/graphql/web/resources/GraphqlCache.java

olim7t · 2022-07-22T17:50:30Z

...aphqlapi/src/test/java/io/stargate/sgv2/graphql/integration/util/GraphqlIntegrationTest.java

+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+
+public abstract class GraphqlIntegrationTest {


For now, this relies on inheritance, which is a bit underwhelming. Requests are executed through the bridge.

I would prefer to port CqlSessionExtension from the v1 codebase, but it's not yet clear to me if/how a Junit extension can interact with a Quarkus test extension.

Yes, we should be able to have the CqlSessionExtension here as well, that communicates over the bridge or maybe even directly over the CQL port.. For this to happen, we need to expose host and port of the target CQL port in the integration tests.. Then you can use Cassandra driver to push needed cql statements..

I might be working on this while you are on holiday.. Let's sync once you are back..

ivansenic

I tried my best to give as much feedback as possible.. I think we should bring to the merging state as soon as possible and then have tickets for future improvements..

General notes:

the api CI workflow was not extended to include sgv2-graphql for the integration tests.. If you disable fixed token in the int-tests, then you can to add your service to be docker-based tested
can we get the set of tickets for improvements, for example removing StargateBridgeClient, async fetchers if possible and going into reactive programming model, etc (you know better)
nit: banner setup is missing

ivansenic · 2022-08-29T13:17:08Z

apis/sgv2-graphqlapi/README.md

-  returns raw JSON responses. See [GraphqlClient] and its subclasses.
-
-If you add new tests, please favor the lightweight approach.
+<!-- TODO integration tests -->


Do we want to introduce a similar guides for building the app, building the docker, int tests, etc as we do for the docs-api.. I guess it would be more a copy & paste.. I think the readme should provide all the information possible.. What about configuration guide as in the docs api?

Instead of copy-pasting, maybe it should move to the top-level readme?

Fine with me, we can even move this to a separate task to speed merging this in. It's low priority anyway.

I agree that we should prioritize merging to the v.2.0.0 branch. That will make it easier to work on the documentation across all 3 of the APIs.

I created #2052.

apis/sgv2-graphqlapi/src/main/java/io/stargate/sgv2/graphql/schema/FileSupport.java

ivansenic · 2022-08-29T13:22:38Z

...ain/java/io/stargate/sgv2/graphql/schema/graphqlfirst/fetchers/deployed/MutationFetcher.java

+      return DEFAULT_PARAMETERS.toBuilder()
          .setConsistency(consistencyLevel)
          .setSerialConsistency(serialConsistencyLevel)
          .build();


should this be read from the configuration now? at least the default values? I am talking about consistency:

stargate/apis/sgv2-quarkus-common/CONFIGURATION.md

Lines 54 to 61 in 03d2bde

### Queries configuration

*Configuration mapping for the query properties, defined by [QueriesConfig.java](src/main/java/io/stargate/sgv2/api/common/config/QueriesConfig.java).*

| Property | Type | Default | Description |

|-----------------------------------------------|----------|----------------|----------------------------------------------------------------------------------|

| `stargate.queries.consistency.schema-changes` | `String` | `LOCAL_QUORUM` | Consistency level to use for C* queries that are performing the schema changes. |

| `stargate.queries.consistency.writes` | `String` | `LOCAL_QUORUM` | Consistency level to use for C* queries that are inserting or updating the data. |

| `stargate.queries.consistency.reads` | `String` | `LOCAL_QUORUM` | Consistency level to use for C* queries that are reading the data. |

Follow-up ticket: #2053

apis/sgv2-graphqlapi/src/main/java/io/stargate/sgv2/graphql/web/resources/AdminResource.java

ivansenic · 2022-08-29T14:01:35Z

...sgv2-quarkus-common/src/test/java/io/stargate/sgv2/common/CqlEnabledIntegrationTestBase.java

+ * <p>Subclasses must be annotated with {@link io.quarkus.test.junit.QuarkusIntegrationTest}.
+ */
+@TestInstance(TestInstance.Lifecycle.PER_CLASS)
+public abstract class CqlEnabledIntegrationTestBase {


cool stuff, can be used in docs api as well.. Do you think in future having CqlSession like stuff as annotation would make sense? Or this is enough?

This can probably be turned into a test resource, now that the contact point info is passed as system properties. The only requirement is that StargateTestResource needs to execute first, do you know if there is a way to enforce a reliable order?

Ok apparently there is QuarkusTestResourceLifecycleManager.order(), I'll give it a try.

Hrmm actually a test resource might not be the best fit:

These resources are started before the first test is run, and are closed at the end of the test suite.

There's no class-level lifecycle that would allow us to create the session and a dedicated keyspace before each test.

Yea I was more thinking about Junit5 extension..

Follow-up ticket: #2054

docker-compose/cassandra-3.11/docker-compose-dev-mode.yml

...qlapi/src/test/java/io/stargate/sgv2/graphql/integration/cqlfirst/ApolloIntegrationTest.java

...aphqlapi/src/test/java/io/stargate/sgv2/graphql/integration/util/GraphqlIntegrationTest.java

apis/sgv2-graphqlapi/src/test/java/io/stargate/sgv2/graphql/integration/util/GraphqlClient.java

ivansenic · 2022-08-30T10:34:11Z

Tried to answer to all the comments.. I'll do a final review on Thursday and we can hopefully merge then.

Since all the unit tests are green now, we should enable the int tests:

Add required maven setup

stargate/apis/sgv2-docsapi/pom.xml

Lines 165 to 190 in ae0f9fd

    
           <plugin> 
        
             <artifactId>maven-failsafe-plugin</artifactId> 
        
             <version>${surefire-plugin.version}</version> 
        
             <executions> 
        
               <execution> 
        
                 <goals> 
        
                   <goal>integration-test</goal> 
        
                   <goal>verify</goal> 
        
                 </goals> 
        
                 <configuration> 
        
                   <systemPropertyVariables> 
        
                     <java.util.logging.manager>org.jboss.logmanager.LogManager</java.util.logging.manager> 
        
                     <maven.home>${maven.home}</maven.home> 
        
                     <testing.containers.cassandra-image>${stargate.int-test.cassandra.image}:${stargate.int-test.cassandra.image-tag}</testing.containers.cassandra-image> 
        
                     <testing.containers.stargate-image>${stargate.int-test.coordinator.image}:${stargate.int-test.coordinator.image-tag}</testing.containers.stargate-image> 
        
                     <testing.containers.cluster-name>${stargate.int-test.cluster.name}</testing.containers.cluster-name> 
        
                     <testing.containers.cluster-version>${stargate.int-test.cluster.version}</testing.containers.cluster-version> 
        
                     <testing.containers.cluster-dse>${stargate.int-test.cluster.dse}</testing.containers.cluster-dse> 
        
                   </systemPropertyVariables> 
        
                   <includes> 
        
                     <include>**/*IntegrationTest.*</include> 
        
                   </includes> 
        
                 </configuration> 
        
               </execution> 
        
             </executions> 
        
           </plugin>

Enable project in CI:

stargate/.github/workflows/apis-v2.yaml

Lines 214 to 215 in ae0f9fd

matrix:

project: [ sgv2-docsapi ]

Enable docker based test:

stargate/.github/workflows/apis-v2.yaml

Lines 321 to 323 in ae0f9fd

    
           - name: Integration Test 
        
             env: 
        
               CONTAINER_TEST: ${{ matrix.project == 'sgv2-docsapi' && 'true' || 'false' }}

olim7t · 2022-08-31T22:12:53Z

apis/sgv2-graphqlapi/pom.xml

@@ -10,6 +10,7 @@
  <properties>
    <quarkus.container-image.group>stargateio</quarkus.container-image.group>
    <quarkus.container-image.name>graphqlapi</quarkus.container-image.name>
+    <quarkus.container-image.tag>v${project.version}</quarkus.container-image.tag>


Our existing code prepends a "v" before the version, whereas Quarkus doesn't. I don't mind either way but if we drop it we'll have to adapt the coordinator and docker-compose scripts.

Can you add this to docs as well.. the release process correctly sets the tag with v, so this would be only for local builds and keeping that docker-compose stuff working

olim7t · 2022-08-31T22:15:14Z

docker-compose/cassandra-3.11/docker-compose.yml

    mem_limit: 2G
    environment:
      - STARGATE_BRIDGE_HOST=coordinator
      - STARGATE_BRIDGE_PORT=8091
-      - STARGATE_GRAPHQL_PORT=9080


We actually don't need to customize the port since the API is in its own container.

you can still set the exposed port with -Dquarkus.http.port if needed..

olim7t · 2022-08-31T22:16:06Z

docker-compose/cassandra-3.11/docker-compose.yml

@@ -75,11 +75,10 @@ services:
    networks:
      - stargate
    ports:
-      - 9080:9080
+      - 8085:8080


I changed the host port because I happen to have something already running on 9080 on my laptop.
Also it aligns better with what is used in the rest of the file.

ivansenic

Had one more look, I would say I have no more comments.. Open discussions should be resolved, tests should be green and we can merge this when @olim7t decides to do it.

Great work +1

ivansenic · 2022-09-01T10:08:54Z

docker-compose/cassandra-3.11/docker-compose-dev-mode.yml

      - STARGATE_BRIDGE_HOST=coordinator
      - STARGATE_BRIDGE_PORT=8091


nit: you can also just set QUARKUS_GRPC_CLIENTS_BRIDGE_HOST here and remove those env vars from docker compose completely..

This reverts commit 9fe3fc9.

olim7t added GraphQL stargate-v2 labels Jul 15, 2022

tatu-at-datastax approved these changes Jul 15, 2022

View reviewed changes

olim7t force-pushed the sgv2-graphqlapi-quarkus branch 2 times, most recently from 5f2aac7 to feddbc0 Compare July 22, 2022 17:31

olim7t commented Jul 22, 2022

View reviewed changes

olim7t force-pushed the sgv2-graphqlapi-quarkus branch from 518cb1c to 908e72c Compare August 11, 2022 00:59

olim7t marked this pull request as ready for review August 12, 2022 17:50

olim7t requested a review from a team as a code owner August 12, 2022 17:50

olim7t force-pushed the sgv2-graphqlapi-quarkus branch from 79ada51 to 688e18e Compare August 12, 2022 21:28

olim7t added 14 commits August 22, 2022 15:47

Convert GraphQL v2 to Quarkus

8ea1c82

Attempt to fix deadlock issue

7c358a8

Port first integration test

0f0277c

Port remaining schema-first integration tests

512343f

Port easy CQL-first tests

16351bb

Port Apollo-based CQL-first tests

b58c4ef

Enable metrics

f127c25

Propagate OVERLOADED as a GraphQL error

875a853

Port log reduction fix (#1279)

3a1e2ff

Remove non-Quarkus GraphQL v2

4a39551

Add Quarkus docker extension

8e51d63

Allow port config via system properties

3778f2c

Create base class for tests that need a CQL session

a8850cf

Migrate all integration tests to QuarkusIntegrationTest

5d23b3d

olim7t force-pushed the sgv2-graphqlapi-quarkus branch from e7836c0 to 5d23b3d Compare August 22, 2022 22:52

olim7t added 2 commits August 26, 2022 11:27

Fix caching issue

bf965e6

Fix parent POM version

d0366ab

ivansenic reviewed Aug 29, 2022

View reviewed changes

olim7t added 2 commits August 29, 2022 11:35

Address misc review comments

a72ba6e

Fix GraphqlResourceBaseTest

7f94548

olim7t added 4 commits August 29, 2022 12:01

Use QuarkusTestResource consistently

14d9d4a

Simplify token handling in PlaygroundResource

a7ce16a

Don't start gRPC server in dev

acda8fc

Drop OkHttp in favor of RestAssured

ec0ae34

olim7t added 5 commits August 30, 2022 14:47

Reenable integration tests

1f9981b

Fix failing tests on DSE

e641553

Fix authentication

ddc61d0

Disable Swagger

75d7b4c

Re-add docker-compose config

08a2f84

olim7t commented Aug 31, 2022

View reviewed changes

Fix missing ks hack

81418f9

ivansenic approved these changes Sep 1, 2022

View reviewed changes

olim7t added 4 commits September 1, 2022 09:13

Fix apollo-runtime dependencies

9fe3fc9

Set log level for integration tests

caf7957

Fix duplicated slashes in test URLs

cb9498e

Revert "Fix apollo-runtime dependencies"

7283c8c

This reverts commit 9fe3fc9.

olim7t mentioned this pull request Sep 2, 2022

Pull default GraphQL CLs from Quarkus configuration #2053

Closed

olim7t merged commit cd0467d into v2.0.0 Sep 2, 2022

olim7t deleted the sgv2-graphqlapi-quarkus branch September 2, 2022 17:23

jeffreyscarpenter mentioned this pull request Jan 22, 2023

Unify configuration in Stargate v2 services #1694

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert GraphQL v2 to Quarkus #1980

Convert GraphQL v2 to Quarkus #1980

olim7t commented Jul 15, 2022 •

edited

Loading

olim7t Jul 22, 2022

ivansenic Jul 25, 2022

ivansenic left a comment •

edited

Loading

ivansenic Aug 29, 2022

olim7t Aug 29, 2022

ivansenic Aug 30, 2022

jeffreyscarpenter Aug 30, 2022

olim7t Sep 2, 2022

ivansenic Aug 29, 2022

olim7t Sep 2, 2022

ivansenic Aug 29, 2022

olim7t Aug 29, 2022

olim7t Aug 29, 2022

olim7t Aug 29, 2022

ivansenic Aug 30, 2022

olim7t Sep 2, 2022

ivansenic commented Aug 30, 2022 •

edited

Loading

olim7t Aug 31, 2022

ivansenic Sep 1, 2022

olim7t Aug 31, 2022

ivansenic Sep 1, 2022

olim7t Aug 31, 2022

ivansenic left a comment

ivansenic Sep 1, 2022

	### Queries configuration
	Configuration mapping for the query properties, defined by [QueriesConfig.java](src/main/java/io/stargate/sgv2/api/common/config/QueriesConfig.java).

	\| Property \| Type \| Default \| Description \|
	\|-----------------------------------------------\|----------\|----------------\|----------------------------------------------------------------------------------\|
	\| `stargate.queries.consistency.schema-changes` \| `String` \| `LOCAL_QUORUM` \| Consistency level to use for C* queries that are performing the schema changes. \|
	\| `stargate.queries.consistency.writes` \| `String` \| `LOCAL_QUORUM` \| Consistency level to use for C* queries that are inserting or updating the data. \|
	\| `stargate.queries.consistency.reads` \| `String` \| `LOCAL_QUORUM` \| Consistency level to use for C* queries that are reading the data. \|

		- STARGATE_BRIDGE_HOST=coordinator
		- STARGATE_BRIDGE_PORT=8091

Convert GraphQL v2 to Quarkus #1980

Convert GraphQL v2 to Quarkus #1980

Conversation

olim7t commented Jul 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivansenic left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivansenic commented Aug 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivansenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olim7t commented Jul 15, 2022 •

edited

Loading

ivansenic left a comment •

edited

Loading

ivansenic commented Aug 30, 2022 •

edited

Loading