You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2020/07/29 07:18:31 UTC
[GitHub] [cassandra] jonmeredith opened a new pull request #698: C15980
jonmeredith opened a new pull request #698:
URL: https://github.com/apache/cassandra/pull/698
See [CASSANDRA-15980](https://issues.apache.org/jira/browse/CASSANDRA-15980#)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] jonmeredith closed pull request #698: CASSANDRA-15980 - Improved log messaging for socket connection/disconnection
Posted by GitBox <gi...@apache.org>.
jonmeredith closed pull request #698:
URL: https://github.com/apache/cassandra/pull/698
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] jonmeredith commented on a change in pull request #698: CASSANDRA-15980 - Improved log messaging for socket connection/disconnection
Posted by GitBox <gi...@apache.org>.
jonmeredith commented on a change in pull request #698:
URL: https://github.com/apache/cassandra/pull/698#discussion_r463709802
##########
File path: src/java/org/apache/cassandra/net/SocketFactory.java
##########
@@ -237,6 +240,30 @@ static String encryptionLogStatement(EncryptionOptions options)
return "enabled (" + encryptionType + ')';
}
+ static String encryptionLogStatement(Channel channel, EncryptionOptions options)
+ {
+ if (options == null)
+ return "disabled";
+
+ StringBuilder sb = new StringBuilder();
+ sb.append("enabled (factory=");
+ sb.append(SSLFactory.openSslIsAvailable() ? "openssl" : "jdk");
+
+ final ChannelPipeline pipeline = channel.pipeline();
Review comment:
Teach me to fix things at the end of the day. Pushed up a fix. The silver lining is that it made me look a bit more carefully about the message and I think the current one is misleading so the fix improves the `toString() output to be more useful.
I've updated the JIRA with more details to get more visibility on it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] dcapwell commented on a change in pull request #698: CASSANDRA-15980 - Improved log messaging for socket connection/disconnection
Posted by GitBox <gi...@apache.org>.
dcapwell commented on a change in pull request #698:
URL: https://github.com/apache/cassandra/pull/698#discussion_r463319155
##########
File path: src/java/org/apache/cassandra/net/SocketFactory.java
##########
@@ -237,6 +239,29 @@ static String encryptionLogStatement(EncryptionOptions options)
return "enabled (" + encryptionType + ')';
}
+ static String encryptionLogStatement(Channel channel, EncryptionOptions options)
+ {
+ if (options == null)
+ return "disabled";
+
+ StringBuilder sb = new StringBuilder();
+ sb.append("enabled (factory=");
+ sb.append(SSLFactory.openSslIsAvailable() ? "openssl" : "jdk");
+
+ SslHandler sslHandler = channel.pipeline().get(SslHandler.class);
Review comment:
you pass null to channel but don't have any checks against it; in testing this patch found a case where this causes a NPE
```
ERROR [main] 2020-07-30 16:00:56,584 CassandraDaemon.java:800 - Exception encountered during startup
java.lang.NullPointerException: null
at org.apache.cassandra.net.SocketFactory.encryptionLogStatement(SocketFactory.java:251)
at org.apache.cassandra.net.InboundConnectionSettings.toString(InboundConnectionSettings.java:87)
at java.base/java.lang.String.valueOf(String.java:2951)
at java.base/java.lang.StringBuilder.append(StringBuilder.java:168)
at org.apache.cassandra.net.InboundConnectionInitiator.bind(InboundConnectionInitiator.java:129)
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] dcapwell commented on a change in pull request #698: CASSANDRA-15980 - Improved log messaging for socket connection/disconnection
Posted by GitBox <gi...@apache.org>.
dcapwell commented on a change in pull request #698:
URL: https://github.com/apache/cassandra/pull/698#discussion_r463357930
##########
File path: src/java/org/apache/cassandra/net/SocketFactory.java
##########
@@ -237,6 +240,30 @@ static String encryptionLogStatement(EncryptionOptions options)
return "enabled (" + encryptionType + ')';
}
+ static String encryptionLogStatement(Channel channel, EncryptionOptions options)
+ {
+ if (options == null)
+ return "disabled";
+
+ StringBuilder sb = new StringBuilder();
+ sb.append("enabled (factory=");
+ sb.append(SSLFactory.openSslIsAvailable() ? "openssl" : "jdk");
+
+ final ChannelPipeline pipeline = channel.pipeline();
Review comment:
this doesn't fix the issue
```
public String toString()
{
return format("address: (%s), nic: %s, encryption: %s",
- bindAddress, FBUtilities.getNetworkInterface(bindAddress.address), SocketFactory.encryptionLogStatement(encryption));
+ bindAddress, FBUtilities.getNetworkInterface(bindAddress.address), SocketFactory.encryptionLogStatement(null, encryption));
}
```
The toString method now passes null as the channel, so the channel itself is null
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] jonmeredith commented on a change in pull request #698: CASSANDRA-15980 - Improved log messaging for socket connection/disconnection
Posted by GitBox <gi...@apache.org>.
jonmeredith commented on a change in pull request #698:
URL: https://github.com/apache/cassandra/pull/698#discussion_r463342965
##########
File path: src/java/org/apache/cassandra/net/SocketFactory.java
##########
@@ -237,6 +239,29 @@ static String encryptionLogStatement(EncryptionOptions options)
return "enabled (" + encryptionType + ')';
}
+ static String encryptionLogStatement(Channel channel, EncryptionOptions options)
+ {
+ if (options == null)
+ return "disabled";
+
+ StringBuilder sb = new StringBuilder();
+ sb.append("enabled (factory=");
+ sb.append(SSLFactory.openSslIsAvailable() ? "openssl" : "jdk");
+
+ SslHandler sslHandler = channel.pipeline().get(SslHandler.class);
Review comment:
fixed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] dcapwell commented on a change in pull request #698: CASSANDRA-15980 - Improved log messaging for socket connection/disconnection
Posted by GitBox <gi...@apache.org>.
dcapwell commented on a change in pull request #698:
URL: https://github.com/apache/cassandra/pull/698#discussion_r463311607
##########
File path: src/java/org/apache/cassandra/net/InboundConnectionInitiator.java
##########
@@ -445,11 +454,11 @@ void setupMessagingPipeline(InetAddressAndPort from, int useMessagingVersion, in
InboundMessageHandler handler =
settings.handlers.apply(from).createHandler(frameDecoder, initiate.type, pipeline.channel(), useMessagingVersion);
- logger.info("{} connection established, version = {}, framing = {}, encryption = {}",
+ logger.info("{} messaging connection established, version = {}, framing = {}, encryption = {}",
Review comment:
before
```
127.0.0.2:7000(127.0.0.1:65115)->127.0.0.1:7000-LARGE_MESSAGES-f53d678a connection established, version = 12, framing = CRC, encryption = enabled (openssl)
```
after
```
127.0.0.2:7000(127.0.0.1:65115)->127.0.0.1:7000-LARGE_MESSAGES-f53d678a messaging connection established, version = 12, framing = CRC, encryption = enabled (openssl)
```
##########
File path: src/java/org/apache/cassandra/net/InboundConnectionInitiator.java
##########
@@ -445,11 +454,11 @@ void setupMessagingPipeline(InetAddressAndPort from, int useMessagingVersion, in
InboundMessageHandler handler =
settings.handlers.apply(from).createHandler(frameDecoder, initiate.type, pipeline.channel(), useMessagingVersion);
- logger.info("{} connection established, version = {}, framing = {}, encryption = {}",
+ logger.info("{} messaging connection established, version = {}, framing = {}, encryption = {}",
Review comment:
personal preference, I prefer the old
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] jonmeredith commented on pull request #698: CASSANDRA-15980 - Improved log messaging for socket connection/disconnection
Posted by GitBox <gi...@apache.org>.
jonmeredith commented on pull request #698:
URL: https://github.com/apache/cassandra/pull/698#issuecomment-671960677
Merged as c9b41c1f8ad03719918c4d3c29719056ae6b3995
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] jonmeredith commented on a change in pull request #698: CASSANDRA-15980 - Improved log messaging for socket connection/disconnection
Posted by GitBox <gi...@apache.org>.
jonmeredith commented on a change in pull request #698:
URL: https://github.com/apache/cassandra/pull/698#discussion_r463329196
##########
File path: src/java/org/apache/cassandra/net/InboundConnectionInitiator.java
##########
@@ -445,11 +454,11 @@ void setupMessagingPipeline(InetAddressAndPort from, int useMessagingVersion, in
InboundMessageHandler handler =
settings.handlers.apply(from).createHandler(frameDecoder, initiate.type, pipeline.channel(), useMessagingVersion);
- logger.info("{} connection established, version = {}, framing = {}, encryption = {}",
+ logger.info("{} messaging connection established, version = {}, framing = {}, encryption = {}",
Review comment:
It's to disambiguate between streaming connections so I'd prefer to keep it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org