[SPARK-4908][SQL]narrow the scope of synchronized for PR 3834

[baishuo]

compared with #3834, this PR narrow the scope of synchronized

[AmplabJenkins]

Can one of the admins verify this patch?

[liancheng]

ok to test

[liancheng]

This LGTM, thanks!

[SparkQA]

Test build #25403 has started for PR 4001 at commit 4bfa306.

• This patch merges cleanly.

[SparkQA]

Test build #25403 has finished for PR 4001 at commit 4bfa306.

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

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25403/
Test PASSed.

[liancheng]

But why this PR is marked as "hotfix"? Does it break anything?

[marmbrus]

Yeah, generally hot fixes are reserved for things like "the build is broken and we need to fix it now"

[baishuo]

Hi @liancheng and @marmbrus I had remove [hotfix] from the title.

[baishuo]

Indeed, the code passed all the test when I do test locally, I add [hotfix] to title just because I want to illustrate that this is not the final solution of [SPARK-4908]

[baishuo]

Hi @marmbrus ,can this PR be merged? :)

[marmbrus]

Do we know that HiveShim.getCommandProcessor( and stuff are threadsafe? Since we only use this for DDL operations and not actually querying this seems more risky than worthwhile to me. Thoughts?

[liancheng]

HiveShim.getCommandProcess delegates to methods defined in CommandProcessorFactory, which tries to find a cached Driver object and initialize it. The underlying Driver cache map is synchronized. However, I'm not quite sure whether Driver is thread-safe. Also, HiveServer2 actually creates a new Driver instance for every SQL statement and never caches them. Considering all the above, I'd agree that the risks is greater than the benefits. A better solution for this is to avoid using HiveShim.getCommandProcess (which caches Driver objects) but mimicing what HiveServer2 does and create new Driver instances for every SQL statement.

[liancheng]

@baishuo Would you mind closing this PR? The Driver.plan field is not properly synchronized; otherwise we wouldn't need the synchronization at all. And other parts of Driver are not obviously thread-safe. Considering DDL operations is infrequent, synchronize the whole function is both safe and performance insensitive.

[baishuo]

no problem,clolse it :)