[SPARK-27868][core] Better default value and documentation for socket server backlog.

[vanzin]

First, there is currently no public documentation for this setting. So it's hard
to even know that it could be a problem if your application starts failing with
weird shuffle errors.

Second, the javadoc attached to the code was incorrect; the default value just uses
the default value from the JRE, which is 50, instead of having an unbounded queue
as the comment implies.

So use a default that is a "rounded" version of the JRE default, and provide
documentation explaining that this value may need to be adjusted. Also added
a log message that was very helpful in debugging an issue caused by this
problem.

[SparkQA]

Test build #105881 has finished for PR 24732 at commit 46b0e73.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[felixcheung]

what's the different between setting to -1 or to 64 as a default? does this change any existing behavior?

[srowen]

Looks like the default previously was effectively 50 from the PR comments

[vanzin]

It's just being explicit, in case there are differences between various JREs. For the most common ones, at least, the default was 50.

[gaborgsomogyi]

LGTM.

[squito]

lgtm

[dongjoon-hyun]

I checked Oracle JDK8 / OpenJDK8 / IBM OpenJ9 JDK8 and additionally Oracle JDK 11. The default is always 50. Shall we use 50 instead of the rounded value 64 conservatively? 64 is better than -1, but I'm not sure we need to change the behavior (50 -> 64) in this PR.

[vanzin]

It's such a small change that it shouldn't affect anybody. (For comparison, the default value on sockets not created from Java is 128.) In fact it makes it clearer that Spark is setting a value explicitly instead of relying on the JVM default (for those who know that detail).

[dongjoon-hyun]

Got it. Thank you, @vanzin . +1, LGTM. Merged to master.

[dongjoon-hyun]

Also, merged to branch-2.4, too.

[dongjoon-hyun]

Thank you for review, @felixcheung , @srowen , @gaborgsomogyi , @squito , too!

[xCASx]

This change, despite being

such a small change that it shouldn't affect anybody

in fact broke our production configuration and led to almost a month of instability of our Spark workflows, while we were investigating the root cause (just because we had a totally different suspect and it took some time to realise that it wasn't a single contributing factor).

In our set up we had the default JVM value overridden, and spark.shuffle.io.backLog left untouched. Of course with the change our set up got broken, because Sparks's new default of 64 got precedence and it is much less than we actually need.

While we learned our own lesson out of this story, I'd like to emphasise, that one should not omit mentioning a change in release notes just because it is believed to be insignificant. Especially if you change names or values of default properties.
Release notes of Spark 2.4.4 has no even a hint of this change.

[vanzin]

Sorry you ran into problems, but I'm curious about this:

In our set up we had the default JVM value overridden

How did you do that? I just looked at the source again and it's hardcoded to 50. Which is why in my mind this wouldn't cause any problems.

[xCASx]

First of all, sorry, "the default JVM value" is just a poor wording from my side. I blindly repeated it from the initial post of this thread. What I meant is OS default.

Here's what it looks like:

org.apache.spark.network.server.TransportServer#init creates NioServerSocketChannel. The channel uses NioServerSocketChannelConfig, which is a successor of io.netty.channel.socket.DefaultServerSocketChannelConfig.
The latter contains two member fields:

protected final ServerSocket javaSocket;
private volatile int backlog = NetUtil.SOMAXCONN;

The first one is in fact a ServerSocket, which defines default value of backlog equal to 50. But this doesn't matter, because even being defined, this value is not used per se.
The real value for backlog comes from the above-mentioned io.netty.channel.socket.DefaultServerSocketChannelConfig#backlog, as you can see it here:

protected void doBind(SocketAddress localAddress) throws Exception {
    if (PlatformDependent.javaVersion() >= 7) {
        javaChannel().bind(localAddress, config.getBacklog());
    } else {
        javaChannel().socket().bind(localAddress, config.getBacklog());
    }
}

And the value of NetUtil.SOMAXCONN is OS dependent.

For EpollServerSocketChannel the logic is similar.

So, the default value in fact neither 50 (I assume, it has never been since Spark migration to netty v4.x), nor even hardcoded (except for Windows it is 200). For Linux the value is taken from /proc/sys/net/core/somaxconn. If the file doesn't exist, it will be resolved from kernel attributes kern.ipc.somaxconn and kern.ipc.soacceptqueue. Only if all mentioned sources aren't defined, the hardcoded value of 128 will be used.

Not only the default value is twice (more than three times for Windows) bigger than 64, but also we forced /proc/sys/net/core/somaxconn to be 8192 long time ago...

[vanzin]

Ah, in that case it might be a better idea to revert the configuration change. Want to open a PR?

[srowen]

Does it make any sense to leave the default but make it higher, if the default can vary a lot and is generally lower than what's practical? (not sure if that's true)
I can see just undoing it too.

[vanzin]

I think that since Netty makes an effort to look at the OS configuration to define the default value, that it makes more sense to not have Spark override that.

[xCASx]

@vanzin will do.

@srowen to be honest, I don't see any flaws in previous implementation (except poor documentation, which is already resolved). Meanwhile doing any assumptions without a context will lead to bigger problems anyway. There is basically no issue which need to be addressed by adjusting the default value. So, let it be -1 as it was before unless there is strong opinion on why it should be changed.

[dongjoon-hyun]

cc @dbtsai , too.

[dongjoon-hyun]

+1 for reverting.

[dongjoon-hyun]

Hi, @xCASx and all.
For RC2 preparation, I revert this commit from branch-2.4 and verified locally first.
Thank you all!

@xCASx . You need to make a reverting PR for master branch only.

[xCASx]

Pull request has been sent. Not sure if this case requires a separate Jira ticket. Reused original SPARK-27868.