-
Notifications
You must be signed in to change notification settings - Fork 28.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-11043][SQL]BugFix:Set the operator log in the thrift server. #9056
Conversation
Test build #43510 has finished for PR 9056 at commit
|
It looks like this overlaps with #9074, by the way. |
It does the same issue. |
@@ -133,6 +133,7 @@ private[hive] class SparkExecuteStatementOperation( | |||
} | |||
|
|||
override def run(): Unit = { | |||
beforeRun() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't forget the afterRun()
Something like?
beforeRun();
try {
runInternal();
} finally {
afterRun();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or as #9074 did, overriding the function runInternal
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel the same, seems that overriding runInternal
directly is a cleaner fix?
Test build #44894 has finished for PR 9056 at commit
|
I had modify the code as you @chenghao-intel comment and also add a simple test case for it @JoshRosen . |
line = reader.readLine() | ||
} | ||
reader.close() | ||
assert(found) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following lines should be enough for this test case.
val expectedLine =
"Operation log root directory is created: " + operationLogPath.getAbsoluteFile
assert(Source.fromFile(logPath).getLines().exists(_.contains(expectedLine)))
Mostly looks good, left two minor comments. |
3f38631
to
d831ccf
Compare
Test build #46487 has finished for PR 9056 at commit
|
Bump @liancheng for final review and sign-off. |
Thanks! Merging to master and branch-1.6. |
`SessionManager` will set the `operationLog` if the configuration `hive.server2.logging.operation.enabled` is true in version of hive 1.2.1. But the spark did not adapt to this change, so no matter enabled the configuration or not, spark thrift server will always log the warn message. PS: if `hive.server2.logging.operation.enabled` is false, it should log the warn message (the same as hive thrift server). Author: huangzhaowei <carlmartinmax@gmail.com> Closes #9056 from SaintBacchus/SPARK-11043. (cherry picked from commit d4a5e6f) Signed-off-by: Cheng Lian <lian@databricks.com>
SessionManager
will set theoperationLog
if the configurationhive.server2.logging.operation.enabled
is true in version of hive 1.2.1.But the spark did not adapt to this change, so no matter enabled the configuration or not, spark thrift server will always log the warn message.
PS: if
hive.server2.logging.operation.enabled
is false, it should log the warn message (the same as hive thrift server).