CASSANALYTICS-130 Support per-instance sidecar port resolution in CDC client#187
CASSANALYTICS-130 Support per-instance sidecar port resolution in CDC client#187Klose6 wants to merge 6 commits intoapache:trunkfrom
Conversation
jberragan
left a comment
jberragan
left a comment
There was a problem hiding this comment.
Thanks for the patch, I made a couple of suggestions.
| protected ClusterConfigProvider clusterConfigProvider; | ||
| protected SidecarCdcClient sidecarCdcClient; | ||
| @NotNull | ||
| protected Function<String, Integer> portResolver; |
There was a problem hiding this comment.
Could you keep a default implementation that returns (ignored) -> config.effectivePort() so we keep the preexisting behavior. Then add a builder method e.g. withPortResolver if users wish to override with their own implementation.
There was a problem hiding this comment.
sure, I added the preexisting behavior in the constructor and another new constructor for the new portResolver parameter, also I just found this latest commit so I add the withPortResolver() in SidecarCdcClient and SidecarCdcBuilder to support the port resolving. Thanks!
There was a problem hiding this comment.
@jberragan I double check the latest commit and think we don't need the changes on SidecarCdcBuilder now because the SidecarCdcClient is a input parameter to SidecarCdc now, remove the withPortResolver() in SidecarCdcBuilder and only the changes in sidecarCdcClient, thanks!
| * Builds a port resolver function from the {@link CdcSidecarInstancesProvider}. The resolver looks up | ||
| * the port for a given hostname from the provider, falling back to the configured effective port. | ||
| */ | ||
| static Function<String, Integer> buildPortResolver(@NotNull CdcSidecarInstancesProvider provider, |
There was a problem hiding this comment.
This is a bit O(n) can we change the interface to be (CassandraInstance) -> Integer, that way we can just return:
(instance) -> instance.port();
There was a problem hiding this comment.
In this latest commit it accepts the sidecarCdcClient in the SidecarCdc.java so I removed this and in cassandra-sidecar we can inject the sidecar port mapping into the sidecarCdcClient with its added withPortResolver() when calls the SidecarCdc.builder().
Klose6
force-pushed
the
CASSANALYTICS-130
branch
from
742ce8a to
347a59c
Compare
3 participants
Currently when the sidecar tries to call another sidecar instance it assumes another sidecar will use the same port as itself, this change make the port as a input parameter so that the downstream caller can have the option to pass in the port if other sidecar is using a different port.