frankgh commented on code in PR #264:
URL: https://github.com/apache/cassandra-sidecar/pull/264#discussion_r2512296639
##########
server/src/main/java/org/apache/cassandra/sidecar/cluster/CassandraAdapterDelegate.java:
##########
@@ -380,6 +411,23 @@ public NodeSettings nodeSettings() throws
CassandraUnavailableException
return nodeSettingsFromJmx;
}
+ /**
+ * Returns the cached node settings read from the system_views.settings
table. These are retrieved after a successful
+ * health check for native transport.
+ * @return Cached node settings from system_view.settings.
+ * @throws CassandraUnavailableException Thrown when native transport is
not available.
+ */
+ @Override
+ @NotNull
+ public Map<String, String> v2NodeSettings() throws
CassandraUnavailableException
+ {
+ if (nodeSettingsFromCql == null)
+ {
+ throw new CassandraUnavailableException(CQL, "CQL NodeSettings
unavailable");
+ }
+ return nodeSettingsFromCql;
Review Comment:
There's a chance for a race here, where one thread is retrieving the
settings and another is updating settings to null in line 322 for example. I
would recommend changing this code to avoid any potential race issues
```suggestion
Map<String, String> settings = nodeSettingsFromCql;
if (settings == null)
{
throw new CassandraUnavailableException(CQL, "CQL NodeSettings
unavailable");
}
return settings;
```
##########
server/src/main/java/org/apache/cassandra/sidecar/cluster/CassandraAdapterDelegate.java:
##########
@@ -242,7 +251,7 @@ protected synchronized void jmxHealthCheck()
/**
* Performs health checks by utilizing the native protocol
*/
- protected void nativeProtocolHealthCheck()
+ protected synchronized void nativeProtocolHealthCheck()
Review Comment:
why is this synchronized? We guarantee a single thread here. Any reason to
change it ?
##########
server/src/main/java/org/apache/cassandra/sidecar/cluster/CassandraAdapterDelegate.java:
##########
@@ -281,6 +291,26 @@ protected void nativeProtocolHealthCheck()
{
notifyNativeConnection();
}
+
+ if (isNativeUp.get())
Review Comment:
this will always be true, I believe. Any case where this might not be true?
--
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]
