jmckenzie-dev commented on code in PR #347:
URL: https://github.com/apache/cassandra-sidecar/pull/347#discussion_r3237038530
##########
server/src/main/java/org/apache/cassandra/sidecar/cdc/CachingSchemaStore.java:
##########
@@ -132,8 +146,15 @@ private void configureSidecarServerEventListeners()
return v;
});
}
- loadPublisher();
- publishSchemas();
+ try
+ {
+ loadPublisher();
+ publishSchemas();
+ }
+ catch (Exception e)
+ {
+ LOGGER.error("Failed to publish schemas to Kafka during
initialization, CDC will still start", e);
Review Comment:
Why do we continue to start even if our schema publishers fail? What was the
thinking here? We should document the "why" here to explain this reasoning.
##########
integration-framework/src/main/java/org/apache/cassandra/sidecar/testing/TestCdcPublisher.java:
##########
@@ -49,30 +47,32 @@ public class TestCdcPublisher extends CdcPublisher
private final CdcDatabaseAccessor databaseAccessor;
public TestCdcPublisher(Vertx vertx,
- SidecarConfiguration sidecarConfiguration,
- ExecutorPools executorPools,
- ClusterConfigProvider clusterConfigProvider,
- SchemaSupplier schemaSupplier,
- CdcSidecarInstancesProvider
sidecarInstancesProvider,
- SidecarCdcClient.ClientConfig clientConfig,
- InstanceMetadataFetcher instanceMetadataFetcher,
- CdcConfig conf,
- CdcDatabaseAccessor databaseAccessor,
- ICdcStats cdcStats,
- VirtualTablesDatabaseAccessor virtualTables,
- SidecarCdcStats sidecarCdcStats,
- Serializer<CdcEvent> avroSerializer,
- Provider<RangeManager> rangeManagerProvider)
+ ExecutorPools executorPools,
+ ClusterConfigProvider clusterConfigProvider,
+ SchemaSupplier schemaSupplier,
+ InstanceMetadataFetcher instanceMetadataFetcher,
+ CdcConfig conf,
+ CdcDatabaseAccessor databaseAccessor,
+ ICdcStats cdcStats,
+ VirtualTablesDatabaseAccessor virtualTables,
+ SidecarCdcStats sidecarCdcStats,
+ Provider<RangeManager> rangeManagerProvider,
+ CassandraBridgeFactory cassandraBridgeFactory,
+ Provider<SidecarCdcClient>
sidecarCdcClientProvider,
+ CdcOptions cdcOptions)
{
- super(vertx, sidecarConfiguration, executorPools,
clusterConfigProvider,
- schemaSupplier, sidecarInstancesProvider, clientConfig,
- instanceMetadataFetcher, conf, databaseAccessor, cdcStats,
- virtualTables, sidecarCdcStats, avroSerializer,
rangeManagerProvider);
+ super(vertx, executorPools, clusterConfigProvider,
+ schemaSupplier, instanceMetadataFetcher, conf, databaseAccessor,
cdcStats,
+ virtualTables, sidecarCdcStats, rangeManagerProvider,
+ cassandraBridgeFactory, sidecarCdcClientProvider,
+ null,
Review Comment:
Passing these 2 as null today may work but it's a landmine for future
modifications. Any changes in the future that try and access the 2 objects that
this is passing in as null will cause things to NPE out in surprising ways in
this test that'll cause trouble in the future. Can we mock in something here
that'll satisfy the API of these 2 objects w/out introducing too much
complexity to prevent that future regression?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
