CASSSIDECAR-208: Guice modularization #200

yifan-c · 2025-02-20T18:07:41Z

Patch by Yifan Cai; Reviewed by TBD for CASSSIDECAR-208

yifan-c · 2025-02-21T03:27:42Z

client-common/src/main/java/org/apache/cassandra/sidecar/common/ApiEndpointsV1.java

@@ -124,7 +124,7 @@ public final class ApiEndpointsV1

    public static final String CONNECTED_CLIENT_STATS_ROUTE = API_V1 + CASSANDRA + "/stats/connected-clients";

-    public static final String OPERATIONAL_JOBS = "/operational-jobs";
+    private static final String OPERATIONAL_JOBS = "/operational-jobs";


this is not the route. It is the intermediate string to define routes that share the same prefix. Such intermediate string does not need to be exposed.

yifan-c · 2025-02-21T03:28:42Z

integration-framework/build.gradle

-    api(project(path: ":server")) {
-        exclude(group: 'org.apache.logging.log4j')
-        exclude(group: 'org.slf4j')
-        exclude(group: 'ch.qos.logback')
-    }


Excluding the dependencies loses the test log of sidecar in the pacakge

yifan-c · 2025-02-21T03:32:30Z

...integrationTest/org/apache/cassandra/sidecar/routes/CassandraSchemaRouteIntegrationTest.java

+        SchemaResponse response = getBlocking(trustedClient()
+                                              .get(server.actualPort(), "localhost", testRoute)
+                                              .send()
+                                              .expecting(HttpResponseExpectation.SC_OK))


expecting is replacing the deprecated expect

yifan-c · 2025-02-21T03:33:10Z

server-common/src/main/java/org/apache/cassandra/sidecar/db/schema/TableSchema.java

+    @Override
+    public String toString()
+    {
+        return keyspaceName() + '.' + tableName();


The string value is used when registering table schema fails

yifan-c · 2025-02-21T03:34:13Z

server/src/main/java/org/apache/cassandra/sidecar/db/schema/SidecarSchema.java

-    /**
-     * Initializer that retries until sidecar schema is initialized. Once initialized, it un-schedules itself.
-     */
-    private class SidecarSchemaInitializer implements PeriodicTask


SidecarSchemaInitializer is moved to its own class file.

yifan-c · 2025-02-21T03:35:05Z

server/src/main/java/org/apache/cassandra/sidecar/db/schema/SystemAuthSchema.java

+    @Override
+    public String toString()
+    {
+        return keyspaceName();


This guy has multiple tables. Therefore showing the keyspace name only.

yifan-c · 2025-02-21T03:37:23Z

server/src/main/java/org/apache/cassandra/sidecar/handlers/RouteBuilder.java

    {
-        Objects.requireNonNull(router, "Router must be set");


router must present now in the VertxRoute lambda

sarankk

Thanks Yifan, modules look neat and well organized. Left some comments

sarankk · 2025-02-27T19:09:22Z

server/src/main/java/org/apache/cassandra/sidecar/db/RestoreSliceDatabaseAccessor.java

                                           CQLSessionProvider cqlSessionProvider)
    {
-        super(restoreSlicesSchema, cqlSessionProvider);
+        super(sidecarSchema.tableSchema(RestoreSlicesSchema.class), cqlSessionProvider);


Nit: should we call this method tableSchemaByClass ?

it is to get the instance using the class to look up.

sarankk · 2025-02-27T19:48:58Z

server/src/main/java/org/apache/cassandra/sidecar/db/schema/SidecarSchemaInitializer.java

+            else if (ex instanceof SidecarSchemaModificationException)
+            {
+                LOGGER.warn("Failed to modify schema", ex);
+                schemaMetrics.failedModifications.metric.update(1);


Since we are initializing, we should use failedInitializations here?

The original implementation is reporting as failedModifications. I would like to keep it as-is.

server/src/main/java/org/apache/cassandra/sidecar/handlers/CassandraHealthHandler.java

sarankk · 2025-02-27T21:56:52Z

server/src/main/java/org/apache/cassandra/sidecar/handlers/RingHandler.java

@@ -16,7 +16,7 @@
 * limitations under the License.
 */

-package org.apache.cassandra.sidecar.routes;
+package org.apache.cassandra.sidecar.handlers;


My only concern with this package name change is, currently we have just API route specific handlers here, other speciality handlers in their respective packages, which keeps handlers more organized. Naming it generic, might lead to all handlers being put here. How about moving handlers package under routes package?

server/src/main/java/org/apache/cassandra/sidecar/routes/VertxRoute.java

sarankk · 2025-03-04T19:41:28Z

server/src/main/java/org/apache/cassandra/sidecar/modules/CassandraOperationsModule.java

+    VertxRoute cassandraSchemaRoute(RouteBuilder.Factory factory,
+                                    SchemaHandler schemaHandler)
+    {
+        return factory.builderForRoute()


Nit: Getting schema information doesn't seem like a operation related. We can leave it as is for now, but later we can move it to CassandraModule or CassandraInfoModule?. Same with ring, token, gossip endpoint

Cassandra related operations, i.e. trigger task or retrieving information, are grouped in the same module. If splitting the module up helps with readability, we can do that in the future.

server/src/main/java/org/apache/cassandra/sidecar/modules/HealthCheckModule.java

sarankk · 2025-03-04T20:02:19Z

server/src/main/java/org/apache/cassandra/sidecar/modules/SidecarModules.java

+                       new AuthModule(),
+                       new CassandraOperationsModule(),
+                       new CdcModule(),
+                       new ConfigurationModule(confPath),


Nit: If we have VertxSetupModule we can move ConfigurationModule after it for easier reading

For this PR, I would like to stick to the idea of group bindings by feature. VertxSetup is not a feature.

Btw, the order of the modules in the list does not affect bindings. They are just sorted alphabetically to look neat.

sarankk · 2025-03-04T21:26:50Z

server/src/main/java/org/apache/cassandra/sidecar/modules/multibindings/KeyClassMapKey.java

+/**
+ * Allows {@literal @}{@link ProvidesIntoMap} to specify a {@link ClassKey} class as map key.
+ * The class must extends from {@link ClassKey}
+ * <p>The annotation name {@link KeyClassMapKey} is read as "map key type is of {@link ClassKey} class"


Nit: Shall we name this class to ClassKeyMapKey ? that way name matches this comment

The full name was ClassKey Class MapKey, map key using the class of ClassKey. There are 2 "Class" in the name, so I dropped one of them. It does not use the ClassKey instance, but its class. Therefore, the later "Class` is reserved.

sarankk · 2025-03-04T21:31:47Z

server/src/main/java/org/apache/cassandra/sidecar/modules/RestoreJobModule.java

+        TypeLiteral<Class<? extends ClassKey>> keyType = new TypeLiteral<>() {};
+        TypeLiteral<PeriodicTask> valueType = new TypeLiteral<>() {};
+        MapBinder<Class<? extends ClassKey>, PeriodicTask> periodicTaskMapBinder = MapBinder.newMapBinder(binder(), keyType, valueType);
+        periodicTaskMapBinder.addBinding(PeriodicTaskMapKeys.RestoreJobDiscovererKey.class).to(RestoreJobDiscoverer.class);


Curious, why do we inject for these periodic tasks and bind them this way. Where as for HealthCheckPeriodicTask we provide with ProvidesIntoMap

They are equivalent. I wanted to demo that there are different ways to add bindings. Plus, binding to a class (that is annotated with @Inject) has simpler code.

Patch by Yifan Cai; Reviewed by TBD for CASSSIDECAR-208

yifan-c force-pushed the CASSSIDECAR-208/guice-modulization branch from 5fbc063 to 74613ae Compare February 21, 2025 03:25

yifan-c commented Feb 21, 2025

View reviewed changes

yifan-c force-pushed the CASSSIDECAR-208/guice-modulization branch 2 times, most recently from 66166a6 to d3f15ac Compare February 23, 2025 00:50

sarankk reviewed Mar 4, 2025

View reviewed changes

yifan-c force-pushed the CASSSIDECAR-208/guice-modulization branch from 55a6e82 to ea31d0e Compare March 8, 2025 06:36

CASSSIDECAR-208: Guice modularization

ec0d834

Patch by Yifan Cai; Reviewed by TBD for CASSSIDECAR-208

yifan-c force-pushed the CASSSIDECAR-208/guice-modulization branch from ea31d0e to ec0d834 Compare March 8, 2025 06:59

yifan-c added 7 commits March 8, 2025 10:48

fix test and add table stats route back

76b5a35

codestyle

dbbff83

fix test

473dffe

fix cdc config routes

3210026

fix update cdc config route

8fcef66

fix test

b597a19

enrich logging

914f875

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CASSSIDECAR-208: Guice modularization #200

CASSSIDECAR-208: Guice modularization #200

yifan-c commented Feb 20, 2025

yifan-c Feb 21, 2025

yifan-c Feb 21, 2025

yifan-c Feb 21, 2025

yifan-c Feb 21, 2025

yifan-c Feb 21, 2025

yifan-c Feb 21, 2025

yifan-c Feb 21, 2025

sarankk left a comment

sarankk Feb 27, 2025

yifan-c Mar 4, 2025

sarankk Feb 27, 2025

yifan-c Mar 4, 2025

sarankk Feb 27, 2025

sarankk Mar 4, 2025

yifan-c Mar 4, 2025

sarankk Mar 4, 2025 •

edited

Loading

yifan-c Mar 5, 2025

sarankk Mar 4, 2025

yifan-c Mar 5, 2025

sarankk Mar 4, 2025

yifan-c Mar 5, 2025 •

edited

Loading

CASSSIDECAR-208: Guice modularization #200

Are you sure you want to change the base?

CASSSIDECAR-208: Guice modularization #200

Conversation

yifan-c commented Feb 20, 2025

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

sarankk left a 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

sarankk Mar 4, 2025 • 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

yifan-c Mar 5, 2025 • edited Loading

Choose a reason for hiding this comment

sarankk Mar 4, 2025 •

edited

Loading

yifan-c Mar 5, 2025 •

edited

Loading