[SPARK-8839][SQL]ThriftServer2 will remove session and execution no matter it's finished or not.

[SaintBacchus]

In my test, sessions and executions in ThriftServer2 is not the same number as the connection number.
For example, if there are 200 clients connecting to the server, but it will have more than 200 sessions and executions.
So if it reaches the retainedStatements, it has to remove some object which is not finished.
So it may cause the exception described in Jira Address

[SparkQA]

Test build #36576 has finished for PR 7239 at commit 9d5ceb8.

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

[SaintBacchus]

Hi, @liancheng will you take a look at this issue?

[liancheng]

@tianyi Could you please help reviewing this one? Thanks!

[tianyi]

@SaintBacchus , thanks for fixing this issue.
In your current commit, the number of object in these lists could NOT be reduced if there are more than toRemove alive objects in the head of the list.
I think you should add a filter before take to pick up and drop finished objects.
But I think there still be a problem if the number of alive object is more than retainedStatements and retainedSessions

[SaintBacchus]

add a filter before take

It's a better idea @tianyi.
I had modified the implement:

1. If there are hundreds of connections, we keep they in memory
2. When each session finished, we trigger trimSessionIfNecessary to remove the finished sessions and keep the list size below retainedStatements

[SparkQA]

Test build #36664 has finished for PR 7239 at commit 3e9a5a6.

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

[liancheng]

@SaintBacchus As for the semantics of the take method, I think it's OK since we are using LinkedHashMap here:

Hash table and linked list implementation of the Map interface, with predictable iteration order. This implementation differs from HashMap in that it maintains a doubly-linked list running through all of its entries. This linked list defines the iteration ordering, which is normally the order in which keys were inserted into the map (insertion-order). Note that insertion order is not affected if a key is re-inserted into the map. (A key k is reinserted into a map m if m.put(k, v) is invoked when m.containsKey(k) would return true immediately prior to the invocation.)

[SaintBacchus]

@liancheng Maybe I mistook this issue, but it actually existed.
The deeper reason I don't mention is that if there are 200 connections at the same time, but the session may be 300 or above. So if we still want to keep the retainedStatements it always will have this issue.

[liancheng]

@SaintBacchus Yeah, I agree there is a bug, just wanted to point out that it's not related to the take method :)

[liancheng]

@SaintBacchus Could you please update the PR description to reflect the actual cause?

[SaintBacchus]

@liancheng I had updated the description.
Now I did not know why the session number will exceed the client number. Do you have any idea?
If we can't avoid this mechanism in Spark Code, my modify may be a temporary solution.

[tianyi]

I think we should only fix the mistake of removing alive sessions or executions in this PR.
If the number of alive objects is more than 200, you should set the spark.sql.thriftserver.ui.retainedStatements and spark.sql.thriftserver.ui.retainedSessions to a bigger value. You should also remember to give more memory to the driver side.

[tianyi]

I think it is not necessary here.

[tianyi]

We already do the checking work when a session opened or an execution started.
The number of these two lists should NOT increase in other phase.

[SaintBacchus]

@tianyi In my solution all the unfinished sessions will keep in memory. If we don't check after session finish, we have to wait a new client to trigger this check.

do the checking work when a session opened or an execution started

[tianyi]

@SaintBacchus yes, I agree that, but I don't understand why we have to remove a closed session immediately. If there are more than 200 session alive, I think there will be more client connecting soon.

[SaintBacchus]

@tianyi reducing a little memory if there is no new client coming soon.

[tianyi]

I agree with you.
Please remove trim in the onStatementParsed, that is really meaningless.
Besides that, @liancheng , this PR now is LGTM

[SaintBacchus]

@tianyi Thanks for review and comment , I had removed it.

[SparkQA]

Test build #36904 has finished for PR 7239 at commit cf7ef40.

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

[SaintBacchus]

@liancheng Can you merge it into master if it's OK?

[liancheng]

LGTM, merging to master. Thanks!