You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2020/11/06 10:07:26 UTC
[GitHub] [ignite] Vladsz83 opened a new pull request #8430: IGNITE-13662 : Discribe soLinger setting in TCP Discovery and SSL issues.
Vladsz83 opened a new pull request #8430:
URL: https://github.com/apache/ignite/pull/8430
Describes `soLinger` setting of `TcpDiscoverySpi`. Notes of knows SSL issues of socket deadlock in SSL configuraton.
----------------------------------------------------------------
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
[GitHub] [ignite] dmagda commented on a change in pull request #8430: IGNITE-13662 : Describe soLinger setting in TCP Discovery and SSL issues.
Posted by GitBox <gi...@apache.org>.
dmagda commented on a change in pull request #8430:
URL: https://github.com/apache/ignite/pull/8430#discussion_r519979370
##########
File path: docs/_docs/clustering/network-configuration.adoc
##########
@@ -56,6 +56,7 @@ You can find the complete list of properties in the javadoc:org.apache.ignite.sp
| `localPort` | The port that the node binds to. If set to a non-default value, other cluster nodes must know this port to be able to discover the node. | `47500`
| `localPortRange`| If the `localPort` is busy, the node attempts to bind to the next port (incremented by 1) and continues this process until it finds a free port. The `localPortRange` property defines the number of ports the node will try (starting from `localPort`).
| `100`
+| `soLinger`| Setting linger-on-close can help with socket deadlocks of SSL issues like JDK-8219658. But costs longer detection of node failure. | `0`
Review comment:
I guess the SSL issue is not the only reason of why developers might want to tweak this flag. How about this description? It's enough to mention the SSL issue on the SSL page only and leave the description in this place generic.
_Specifies a linger-on-close timeout. This option disables/enables immediate return from a close() of a TCP Socket. Setting the timeout to `0` closes the socket immediately. See the Javadoc for more details._
##########
File path: docs/_docs/security/ssl-tls.adoc
##########
@@ -32,6 +32,12 @@ To enable SSL/TLS for cluster nodes, configure an `SSLContext` factory in the no
You can use the `org.apache.ignite.ssl.SslContextFactory`, which is the default factory that uses a configurable keystore to initialize the SSL context.
//You can also implement your own `SSLContext` factory.
+[NOTE]
+====
Review comment:
I would put it this way:
_Consider setting the `TcpDiscovery.soLinger` parameter to X to avoid SSL-related deadlocks on certain versions of JRE. Alternatively, update your JRE version to the latest one._
And do the following with this text:
1. Turn the "SSL-related deadlock" phrase into an external link: https://github.com/apache/ignite/tree/master/docs#links-to-external-resources
2. The callout needs to be of the "caution" type: https://github.com/apache/ignite/tree/master/docs#callouts
----------------------------------------------------------------
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
[GitHub] [ignite] Vladsz83 commented on a change in pull request #8430: IGNITE-13662 : Describe soLinger setting in TCP Discovery and SSL issues.
Posted by GitBox <gi...@apache.org>.
Vladsz83 commented on a change in pull request #8430:
URL: https://github.com/apache/ignite/pull/8430#discussion_r520473812
##########
File path: docs/_docs/clustering/network-configuration.adoc
##########
@@ -56,6 +56,7 @@ You can find the complete list of properties in the javadoc:org.apache.ignite.sp
| `localPort` | The port that the node binds to. If set to a non-default value, other cluster nodes must know this port to be able to discover the node. | `47500`
| `localPortRange`| If the `localPort` is busy, the node attempts to bind to the next port (incremented by 1) and continues this process until it finds a free port. The `localPortRange` property defines the number of ports the node will try (starting from `localPort`).
| `100`
+| `soLinger`| Setting linger-on-close can help with socket deadlocks of SSL issues like JDK-8219658. But costs longer detection of node failure. | `0`
Review comment:
I don't think so because socket is an internal implementation of the SPI. User shouldn't know about it. As example, there is no linger option for the communication SPI. Its internals are NIO. Linger was introduced only to have a workaround for SSL deadlock issues. It seems there was no need and are no need to configure 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
[GitHub] [ignite] dmagda commented on a change in pull request #8430: IGNITE-13662 : Describe soLinger setting in TCP Discovery and SSL issues.
Posted by GitBox <gi...@apache.org>.
dmagda commented on a change in pull request #8430:
URL: https://github.com/apache/ignite/pull/8430#discussion_r521048510
##########
File path: docs/_docs/clustering/network-configuration.adoc
##########
@@ -56,6 +56,7 @@ You can find the complete list of properties in the javadoc:org.apache.ignite.sp
| `localPort` | The port that the node binds to. If set to a non-default value, other cluster nodes must know this port to be able to discover the node. | `47500`
| `localPortRange`| If the `localPort` is busy, the node attempts to bind to the next port (incremented by 1) and continues this process until it finds a free port. The `localPortRange` property defines the number of ports the node will try (starting from `localPort`).
| `100`
+| `soLinger`| Setting linger-on-close can help with socket deadlocks of SSL issues like JDK-8219658. But costs longer detection of node failure. | `0`
Review comment:
Moreover, you already have this workaround mentioned on the SSL page below that is a proper place. To recap, we need a generic description of this soLinger parameter on this page and have the workaround added as a warning callout on the SSL page.
----------------------------------------------------------------
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
[GitHub] [ignite] Vladsz83 commented on pull request #8430: IGNITE-13662 : Describe soLinger setting in TCP Discovery and SSL issues.
Posted by GitBox <gi...@apache.org>.
Vladsz83 commented on pull request #8430:
URL: https://github.com/apache/ignite/pull/8430#issuecomment-726226422
@dmagda , new doc improvement PR: https://github.com/apache/ignite/pull/8452 . Please review. Didn't only manage to make a caution within the table.
----------------------------------------------------------------
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
[GitHub] [ignite] dmagda commented on a change in pull request #8430: IGNITE-13662 : Describe soLinger setting in TCP Discovery and SSL issues.
Posted by GitBox <gi...@apache.org>.
dmagda commented on a change in pull request #8430:
URL: https://github.com/apache/ignite/pull/8430#discussion_r521359420
##########
File path: docs/_docs/clustering/network-configuration.adoc
##########
@@ -56,6 +56,7 @@ You can find the complete list of properties in the javadoc:org.apache.ignite.sp
| `localPort` | The port that the node binds to. If set to a non-default value, other cluster nodes must know this port to be able to discover the node. | `47500`
| `localPortRange`| If the `localPort` is busy, the node attempts to bind to the next port (incremented by 1) and continues this process until it finds a free port. The `localPortRange` property defines the number of ports the node will try (starting from `localPort`).
| `100`
+| `soLinger`| Setting linger-on-close can help with socket deadlocks of SSL issues like JDK-8219658. But costs longer detection of node failure. | `0`
Review comment:
The value of the parameter [is passed into the Socket via its Java API](https://github.com/apache/ignite/blob/master/modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/TcpDiscoverySpi.java#L1687), thus the description on this page needs to sound generic like in Java. Our [JavaDoc follows this guideline](https://ignite.apache.org/releases/latest/javadoc/org/apache/ignite/spi/discovery/tcp/TcpDiscoverySpi.html#setSoLinger-int-) and doesn't mention any workarounds for specific use cases.
We can put everything together this way mentioning the SSL issue as a reason why this parameter set to 0 for Ignite discovery sockets:
_Specifies a linger-on-close timeout. This option disables/enables immediate return from a close() of a TCP Socket. The option defaults to `0` for Ignite TcpDiscovery sockets to avoid [potential deadlocks with SSL connections](https://bugs.openjdk.java.net/browse/JDK-8219658)._
Please take this all into consideration and send another pull-request. We've already beat this to death. The initial pull-request was merged skipping a review of technical documentation committers and maintainers.
----------------------------------------------------------------
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
[GitHub] [ignite] dmagda commented on a change in pull request #8430: IGNITE-13662 : Describe soLinger setting in TCP Discovery and SSL issues.
Posted by GitBox <gi...@apache.org>.
dmagda commented on a change in pull request #8430:
URL: https://github.com/apache/ignite/pull/8430#discussion_r519986691
##########
File path: docs/_docs/security/ssl-tls.adoc
##########
@@ -32,6 +32,12 @@ To enable SSL/TLS for cluster nodes, configure an `SSLContext` factory in the no
You can use the `org.apache.ignite.ssl.SslContextFactory`, which is the default factory that uses a configurable keystore to initialize the SSL context.
//You can also implement your own `SSLContext` factory.
+[NOTE]
+====
Review comment:
I would put it this way:
_Consider setting the `TcpDiscovery.soLinger` parameter to X to avoid SSL-related deadlocks on certain versions of JRE. Alternatively, update your JRE version to the latest one._
And do the following with this text:
1. Turn the "SSL-related deadlock" phrase into an external link to the JDK ticket: https://github.com/apache/ignite/tree/master/docs#links-to-external-resources
2. The callout needs to be of the "caution" type: https://github.com/apache/ignite/tree/master/docs#callouts
----------------------------------------------------------------
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
[GitHub] [ignite] dmagda commented on a change in pull request #8430: IGNITE-13662 : Describe soLinger setting in TCP Discovery and SSL issues.
Posted by GitBox <gi...@apache.org>.
dmagda commented on a change in pull request #8430:
URL: https://github.com/apache/ignite/pull/8430#discussion_r520867019
##########
File path: docs/_docs/clustering/network-configuration.adoc
##########
@@ -56,6 +56,7 @@ You can find the complete list of properties in the javadoc:org.apache.ignite.sp
| `localPort` | The port that the node binds to. If set to a non-default value, other cluster nodes must know this port to be able to discover the node. | `47500`
| `localPortRange`| If the `localPort` is busy, the node attempts to bind to the next port (incremented by 1) and continues this process until it finds a free port. The `localPortRange` property defines the number of ports the node will try (starting from `localPort`).
| `100`
+| `soLinger`| Setting linger-on-close can help with socket deadlocks of SSL issues like JDK-8219658. But costs longer detection of node failure. | `0`
Review comment:
The linger setting is a TCP parameter that changes how the protocol behaves. Even if the initial motivation for adding it to the discovery SPI was to address the SSL issue, the setting still needs to be explained generically because it helps you to control the TCP behavior for all sorts of needs.
The workarounds are usually handled via system properties in Ignite. As long as you've already added a new API method to the public interface it needs to be explained generically. Otherwise, you can remove the linger setting from the API and create a system property for the workaround instead.
----------------------------------------------------------------
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
[GitHub] [ignite] Vladsz83 commented on a change in pull request #8430: IGNITE-13662 : Describe soLinger setting in TCP Discovery and SSL issues.
Posted by GitBox <gi...@apache.org>.
Vladsz83 commented on a change in pull request #8430:
URL: https://github.com/apache/ignite/pull/8430#discussion_r521307187
##########
File path: docs/_docs/clustering/network-configuration.adoc
##########
@@ -56,6 +56,7 @@ You can find the complete list of properties in the javadoc:org.apache.ignite.sp
| `localPort` | The port that the node binds to. If set to a non-default value, other cluster nodes must know this port to be able to discover the node. | `47500`
| `localPortRange`| If the `localPort` is busy, the node attempts to bind to the next port (incremented by 1) and continues this process until it finds a free port. The `localPortRange` property defines the number of ports the node will try (starting from `localPort`).
| `100`
+| `soLinger`| Setting linger-on-close can help with socket deadlocks of SSL issues like JDK-8219658. But costs longer detection of node failure. | `0`
Review comment:
> The linger setting is a TCP parameter that changes how the protocol behaves.
linger only sets socket behavior on socket closing with rest of the data in TCP buffers. But this doesn’t affect behavior of Discovery because it has own message acknowledgement. The exception is the delay of Discovery. That is why I think duplicating of Java API documentation is useless for Ignite user. This parameter doesn’t change general logic and gives no benefits to user.
> The workarounds are usually handled via system properties in Ignite. As long as you've already added a new API method to the public interface .
I haven't. This API had been added in 2.8 as hotfix of raised SSL issue. But hadn't been documented. We just changed its default value to disabled like before 2.8.
----------------------------------------------------------------
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
[GitHub] [ignite] anton-vinogradov merged pull request #8430: IGNITE-13662 : Describe soLinger setting in TCP Discovery and SSL issues.
Posted by GitBox <gi...@apache.org>.
anton-vinogradov merged pull request #8430:
URL: https://github.com/apache/ignite/pull/8430
----------------------------------------------------------------
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
[GitHub] [ignite] Vladsz83 commented on a change in pull request #8430: IGNITE-13662 : Describe soLinger setting in TCP Discovery and SSL issues.
Posted by GitBox <gi...@apache.org>.
Vladsz83 commented on a change in pull request #8430:
URL: https://github.com/apache/ignite/pull/8430#discussion_r521307187
##########
File path: docs/_docs/clustering/network-configuration.adoc
##########
@@ -56,6 +56,7 @@ You can find the complete list of properties in the javadoc:org.apache.ignite.sp
| `localPort` | The port that the node binds to. If set to a non-default value, other cluster nodes must know this port to be able to discover the node. | `47500`
| `localPortRange`| If the `localPort` is busy, the node attempts to bind to the next port (incremented by 1) and continues this process until it finds a free port. The `localPortRange` property defines the number of ports the node will try (starting from `localPort`).
| `100`
+| `soLinger`| Setting linger-on-close can help with socket deadlocks of SSL issues like JDK-8219658. But costs longer detection of node failure. | `0`
Review comment:
> The linger setting is a TCP parameter that changes how the protocol behaves.
linger only sets socket behavior on socket closing with rest of the data in TCP buffers. But this doesn’t affect behavior of Discovery because it has own message acknowledgement. The exception is the delay of Discovery. That is why I think duplicating of Java API documentation is useless for Ignite user. This parameter doesn’t change general logic and gives no benefits to user.
> The workarounds are usually handled via system properties in Ignite. As long as you've already added a new API method to the public interface .
I haven't. This API had been added in 2.8 as hotfix of raised SSL issue. But hadn't been documented. We just changed it's default value to disabled like before 2.8.
----------------------------------------------------------------
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