[SPARK-21395][SQL] Spark SQL hive-thriftserver doesn't register operation log before execute sql statement

[debugger87]

What changes were proposed in this pull request?

• Add a private method registerCurrentOperationLog in SparkExecuteStatementOperation
• Call registerCurrentOperationLog before execute() and unregister it after execute() like implementation in SQLOperation.java#L204

How was this patch tested?

Add unittest in HiveThriftBinaryServerSuite

build/mvn -Phive-thriftserver -Dtest=none -DwildcardSuites=org.apache.spark.sql.hive.thriftserver.HiveThriftBinaryServerSuite test

HiveThriftBinaryServerSuite tests passed

[debugger87]

@cloud-fan Could you please help me to review this PR? Thanks a lot!

[cloud-fan]

ok to test

[SparkQA]

Test build #79657 has finished for PR 18649 at commit b5c963f.

• This patch fails Scala style tests.
• This patch merges cleanly.
• This patch adds no public classes.

[debugger87]

@cloud-fan I just fix scala style issue, pls re-test again.

[SparkQA]

Test build #79659 has finished for PR 18649 at commit fa4ba3d.

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

[debugger87]

@cloud-fan Any suggestions?

[cloud-fan]

cc @jerryshao

[jerryshao]

So it looks like a basic copy-paste from hive code to support this feature in STS (like what I did before to support spnego). It looks fine to me based on my limited knowledge :).

[debugger87]

@jerryshao Yes, it's just copied from SQLOperation in Hive. However, those code lines are the key point that HiveServer2 can return operation log to client via TFetchResultsReq which fetchType is FetchType.LOG. If hive-thriftserver in Spark SQL support complete HiveServer2 protocol, registerCurrentOperationLog should be introduced and consistent with Hive.

[jerryshao]

Sorry I'm not familiar with this part, I cannot give you a valid comment, you could ask others to help reviewing your patch 😄 .

[debugger87]

@gatorsmile Could you please help me to review this PR?

[debugger87]

@cloud-fan This patch is very simple and clear, is there any problem to review or merge it?

[cloud-fan]

I'm not familiar with this code and don't know who I should ping, cc @srowen do we have a maintainer for thrift-server?

[debugger87]

@cloud-fan @srowen If we can't find maintainer of hive-thriftserver in Spark, I may have to close this PR in few days later.

[cloud-fan]

CC @vanzin

[vanzin]

Really not familiar with this area of the code...

[debugger87]

cc @davies

[dilipbiswal]

@debugger87 Hello,
There have been a few posts about hive, where this setting causes too many open files issue.
https://community.hortonworks.com/questions/48351/hiveserver2-hive-users-nofile-ulimit-above-64000.html
there are a few concerns on this setting causing too many open file handles..
Does this also impact spark the same way or we have taken care of it already.

[debugger87]

@dilipbiswal

Thanks for your reply!
In my eyes, there have been some mechanism or configuration to control the number of opening files generated by SQL Operation. e.g:

hive.server2.idle.session.check.operation
hive.server2.session.check.interval
hive.server2.idle.session.timeout
hive.server2.idle.operation.timeout
hive.server2.logging.operation.enabled

If we mind the increase of opening files, we can adjust those configurations. However, in spark sql, there is no any methods or solutions to generate operation logs to provide the standard API: FetchResults with FetchType.LOG.

[SparkQA]

Test build #80744 has finished for PR 18649 at commit 542e427.

• This patch fails Scala style tests.
• This patch merges cleanly.
• This patch adds no public classes.

[gatorsmile]

@liufengdb

[AmplabJenkins]

Can one of the admins verify this patch?

[debugger87]

#19721 Fixed the same issue, i will close it.