CASSANALYTICS-142 : SidecarCdcClient should be passed as a constructor parameter for SidecarCdc to avoid thread/resource leaks#188
Conversation
…r parameter for SidecarCdc to avoid thread/resource leaks
| super(jobId, partitionId, eventConsumer, schemaSupplier); | ||
| this.clusterConfigProvider = clusterConfigProvider; | ||
| this.sidecarCdcClient = new SidecarCdcClient(clientConfig, sidecarInstancesProvider, secretsProvider, cdcStats); | ||
| withCdcOptions(cdcOptions); | ||
| withTokenRangeSupplier(tokenRangeSupplier); | ||
| } |
There was a problem hiding this comment.
You should be able to reuse the other constructor.
this(jobId, partitionId, cdcOptions, clusterConfigProvider, eventConsumer, schemaSupplier, tokenRangeSupplier,
new SidecarCdcClient(clientConfig, sidecarInstancesProvider, secretsProvider, cdcStats),
cdcStats);However, the real question is: why do we need the constructor that creates SidecarCdcClient? We want to share the global SidecarCdcClient instance.
There was a problem hiding this comment.
We want the SidecarCdcClient object to be singleton created in the Sidecar process and it should be shared across all the SidecarCdc instances which are CDC iterators.
I have added this new constructor to take sidecarCdcClient object as parameter so that we can have singleton object and can pass it to all iterators to avoid creating one sidecarCdcClient instance per iterator.
If we have the SidecarCdcClient creation inside the constructor, then on token range change when new iterator is created a new SidecarCdcClient is created, which we want to avoid.
...a-analytics-cdc-sidecar/src/main/java/org/apache/cassandra/cdc/sidecar/SidecarCdcClient.java
Show resolved
Hide resolved
| @@ -75,6 +75,27 @@ public static SidecarCdcBuilder builder(@NotNull String jobId, | |||
| cdcStats); | |||
| } | |||
There was a problem hiding this comment.
Do we still need this build? Please remove it if it is no longer needed. One reason for removal is that the builder path creates a new sidecar cdc client.
There was a problem hiding this comment.
good catch, this is no longer needed and we should remove it as the builder path creates new SidecarCdcClient.
I also think, we should remove the constructor which creates new SidecarCdcClient in the constructor.
No description provided.