jmckenzie-dev commented on code in PR #315:
URL: https://github.com/apache/cassandra-sidecar/pull/315#discussion_r2794238485
##########
server/src/main/java/org/apache/cassandra/sidecar/cdc/CdcPublisher.java:
##########
@@ -165,27 +153,20 @@ public SecretsProvider secretsProvider()
Map<String, String> sslConfigMap = new HashMap<>();
- sslConfigMap.put(SSL_ENABLED_KEY, sslConfiguration.enabled() + "");
- sslConfigMap.put(SSL_PREFER_OPENSSL_KEY,
sslConfiguration.preferOpenSSL() + "");
- sslConfigMap.put(SSL_CLIENT_AUTH_KEY, sslConfiguration.clientAuth());
- sslConfigMap.put(SSL_CIPHER_SUITES_KEY, String.join(",",
sslConfiguration.cipherSuites()));
- sslConfigMap.put(SSL_SECURE_TRANSPORT_PROTOCOLS_KEY, String.join(",",
sslConfiguration.secureTransportProtocols()));
- sslConfigMap.put(SSL_HANDSHAKE_TIMEOUT_KEY,
sslConfiguration.handshakeTimeout().toString());
-
if (sslConfiguration.isKeystoreConfigured())
{
KeyStoreConfiguration keystore = sslConfiguration.keystore();
- sslConfigMap.put(SSL_KEYSTORE_PATH_KEY, keystore.path());
- sslConfigMap.put(SSL_KEYSTORE_PASSWORD_KEY, keystore.password());
- sslConfigMap.put(SSL_KEYSTORE_TYPE_KEY, keystore.type());
+ sslConfigMap.put(MapUtils.lowerCaseKey(SslConfig.KEYSTORE_PATH),
keystore.path());
Review Comment:
Might be worth adding a comment here as to why we're moving things lowercase
like this since we have a split in locality between 2 codebases w/this
formatting dependency. And/or just use the CaseInsensitiveStringMap on our
`sslConfigMap` here if you wanted to. 🤷🏻
Not a hard requirement at all - PR is good the way it is. Just trying to
think through how to simplify things.
##########
server/src/main/java/org/apache/cassandra/sidecar/cdc/CdcPublisher.java:
##########
@@ -165,27 +153,20 @@ public SecretsProvider secretsProvider()
Map<String, String> sslConfigMap = new HashMap<>();
- sslConfigMap.put(SSL_ENABLED_KEY, sslConfiguration.enabled() + "");
- sslConfigMap.put(SSL_PREFER_OPENSSL_KEY,
sslConfiguration.preferOpenSSL() + "");
- sslConfigMap.put(SSL_CLIENT_AUTH_KEY, sslConfiguration.clientAuth());
- sslConfigMap.put(SSL_CIPHER_SUITES_KEY, String.join(",",
sslConfiguration.cipherSuites()));
- sslConfigMap.put(SSL_SECURE_TRANSPORT_PROTOCOLS_KEY, String.join(",",
sslConfiguration.secureTransportProtocols()));
- sslConfigMap.put(SSL_HANDSHAKE_TIMEOUT_KEY,
sslConfiguration.handshakeTimeout().toString());
-
if (sslConfiguration.isKeystoreConfigured())
{
KeyStoreConfiguration keystore = sslConfiguration.keystore();
- sslConfigMap.put(SSL_KEYSTORE_PATH_KEY, keystore.path());
- sslConfigMap.put(SSL_KEYSTORE_PASSWORD_KEY, keystore.password());
- sslConfigMap.put(SSL_KEYSTORE_TYPE_KEY, keystore.type());
+ sslConfigMap.put(MapUtils.lowerCaseKey(SslConfig.KEYSTORE_PATH),
keystore.path());
Review Comment:
My own ignorance here - why do we `lowerCaseKey` them here? Asking claude
indicates Spark's DataSource V2 API uses CaseInsensitiveStringMap which tracks
from poking around at that code. Makes me wonder why we have all the
`MapUtils.java` stuff in analytics instead of just using
[CaseInsensitiveStringMap](https://commons.apache.org/proper/commons-collections/apidocs/org/apache/commons/collections4/map/CaseInsensitiveMap.html)
ourselves...
Certainly beyond the scope of this patch but might be worth some follow up
thought if it'd clean up the codebase.
--
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]
