Github user liancheng commented on the pull request:
https://github.com/apache/spark/pull/2671#issuecomment-58022609
@scwf Thanks, this is a good catch. However, I should mention that
HiveThriftServer2Suite is known to be flaky before the Thrift server is made a
daemon. I had opened #2214 to try to fix this issue, but unfortunately Jenkins
fails because of unknown reason that I couldn't reproduce locally. After
numerous unsuccessful tries, I haven't got time to get it done yet. Sorry for
the trouble... The essential issue fixed in #2214 is that the exception caught
in HiveThriftServer2Suite is not re-thrown in the `catch` clause. That's why it
always passes no matter what exception is thrown.
Back to this PR, I have several comments:
1. Personally I don't prefer to start/stop the server process in
`beforeAll`/`afterAll`. I'd like to make sure every test is executed against a
Thrift server process with clean states.
1. The sleeps introduced in this PR can be eliminated by starting a `tail`
process to watch the log file, and then monitor the output of the `tail`
process. Since an empty log file does no harm, we can try to create a new empty
log file to ensure the file exists before executing `tail`.
1. Log file should be removed after stopping the server process.
Since the Jenkins failure issue in #2214 is really tricky to fix and
without fixing that, we couldn't make any change to `HiveThriftServer2Suite`,
I'm going to open new PR to have another try to fix this issue together with
those left in #2214.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
