yifan-c commented on code in PR #189:
URL: https://github.com/apache/cassandra-sidecar/pull/189#discussion_r1970631585
##########
server/src/main/java/org/apache/cassandra/sidecar/utils/SidecarClientProvider.java:
##########
@@ -76,6 +77,21 @@ public SidecarClient get()
return client;
}
+ public void close(Promise<Void> completion)
+ {
+ LOGGER.info("Closing Sidecar Client...");
+ try
+ {
+ client.close();
+ Thread.sleep(100);
Review Comment:
This (sleep) is interesting. Why?
##########
server/src/main/java/org/apache/cassandra/sidecar/coordination/InnerDcTokenAdjacentPeerProvider.java:
##########
@@ -66,42 +63,33 @@ public class InnerDcTokenAdjacentPeerProvider implements
SidecarPeerProvider
{
private static final Logger LOGGER =
LoggerFactory.getLogger(InnerDcTokenAdjacentPeerProvider.class);
- protected final InstancesMetadata instancesMetadata;
+ protected final InstanceMetadataFetcher instanceFetcher;
private final CassandraClientTokenRingProvider
cassandraClientTokenRingProvider;
private final ServiceConfiguration serviceConfiguration;
private final DnsResolver dnsResolver;
@Inject
- public InnerDcTokenAdjacentPeerProvider(InstancesMetadata
instancesMetadata,
+ public InnerDcTokenAdjacentPeerProvider(InstanceMetadataFetcher
instanceFetcher,
CassandraClientTokenRingProvider
cassandraClientTokenRingProvider,
ServiceConfiguration
serviceConfiguration,
DnsResolver dnsResolver)
{
- this.instancesMetadata = instancesMetadata;
+ this.instanceFetcher = instanceFetcher;
this.cassandraClientTokenRingProvider =
cassandraClientTokenRingProvider;
this.serviceConfiguration = serviceConfiguration;
this.dnsResolver = dnsResolver;
}
public Set<SidecarInstance> get()
{
- Map<Integer, InstanceMetadata> localInstances = instancesMetadata
- .instances()
Review Comment:
Please drop the `final`s in this method
##########
client/src/main/java/org/apache/cassandra/sidecar/client/SidecarClient.java:
##########
@@ -114,6 +115,22 @@ public CompletableFuture<HealthResponse> sidecarHealth()
.build());
}
+ /**
+ * Executes the Sidecar health request using the configured selection
policy and with no retries
+ *
+ * @return a completable future of the Sidecar health response
+ */
+ public CompletableFuture<HealthResponse> peerHealth(SidecarInstance
instance)
+ {
+ return executor.executeRequestAsync(requestBuilder()
+ .singleInstanceSelectionPolicy(
+ new
SidecarInstanceImpl(instance.hostname(),
+
instance.port()))
+
.retryPolicy(oncePerInstanceRetryPolicy)
+ .sidecarHealthRequest()
+ .build());
Review Comment:
not aligned properly.
```suggestion
return executor.executeRequestAsync(requestBuilder()
.singleInstanceSelectionPolicy(instance)
.retryPolicy(oncePerInstanceRetryPolicy)
.sidecarHealthRequest()
.build());
```
##########
server/src/main/java/org/apache/cassandra/sidecar/utils/SidecarClientProvider.java:
##########
@@ -76,6 +77,21 @@ public SidecarClient get()
return client;
}
+ public void close(Promise<Void> completion)
+ {
+ LOGGER.info("Closing Sidecar Client...");
+ try
+ {
+ client.close();
+ Thread.sleep(100);
+ completion.complete();
+ }
+ catch (Throwable throwable)
+ {
+ completion.fail(throwable);
+ }
+ }
Review Comment:
This is not really async. Let's not declare promise input. See the
suggestion below, and at the callsite, i.e. `Server#close`, have this instead.
```
Future.future(p -> {
sidecarClientProvider.close();
p.complete();
});
```
```suggestion
private final AtomicBoolean isClosing = new AtomicBoolean(false);
public void close()
{
if (isClosing.compareAndSet(false, true))
{
LOGGER.info("Closing Sidecar Client...");
ThrowableUtils.propagate(client::close);
}
}
```
##########
server/src/main/java/org/apache/cassandra/sidecar/coordination/SidecarPeerHealthMonitorTask.java:
##########
@@ -67,19 +65,13 @@ public SidecarPeerHealthMonitorTask(Vertx vertx,
SidecarConfiguration
sidecarConfiguration,
SidecarPeerProvider
sidecarPeerProvider,
SidecarPeerHealthProvider
healthProvider,
- SidecarInstanceCodecs
sidecarInstanceCodecs)
+ SidecarInstanceCodec
sidecarInstanceCodec)
{
this.vertx = vertx;
this.config = sidecarConfiguration.sidecarPeerHealthConfiguration();
this.sidecarPeerProvider = sidecarPeerProvider;
this.healthProvider = healthProvider;
- vertx.eventBus().registerDefaultCodec(SidecarInstance.class,
sidecarInstanceCodecs);
- }
-
- @NotNull
- public SidecarPeerHealthProvider.Health status(InstanceMetadata instance)
- {
- return status.getOrDefault(instance,
SidecarPeerHealthProvider.Health.UNKNOWN);
+ vertx.eventBus().registerDefaultCodec(SidecarInstance.class,
sidecarInstanceCodec);
Review Comment:
nit: it is not the best place to register codec for types. For now, it is
fine. Maybe comment a todo above this line.
--
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: pr-unsubscr...@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscr...@cassandra.apache.org
For additional commands, e-mail: pr-h...@cassandra.apache.org
