Skip to content

DRILL-7056: Drill fails with NPE when starting in distributed mode & …#1656

Closed
kkhatua wants to merge 6 commits intoapache:masterfrom
kkhatua:DRILL-7056
Closed

DRILL-7056: Drill fails with NPE when starting in distributed mode & …#1656
kkhatua wants to merge 6 commits intoapache:masterfrom
kkhatua:DRILL-7056

Conversation

@kkhatua
Copy link
Contributor

@kkhatua kkhatua commented Feb 25, 2019

…31010 port is used

This occurs because during the WebServer.close() , one of the tasks for clean up is to delete the temp directory which never existed. This PR's fix does a check for that.

…31010 port is used

This occurs because during the WebServer.close() , one of the tasks for clean up is to delete the temp directory which never existed. This PR's fix does a check for that.
@kkhatua kkhatua self-assigned this Feb 25, 2019
@kkhatua kkhatua requested a review from vvysotskyi February 25, 2019 17:16
@vvysotskyi
Copy link
Member

@kkhatua, please add the unit test (you may use TestGracefulShutdown.testDrillbitWithSamePortContainsShutdownThread as an example).

Also, in WebServer tmpJavaScriptDir is marked for deletion (tmpJavaScriptDir.deleteOnExit()) and this directory is also deleted in the close() method. Please either remove deleteOnExit() call or remove FileUtils.deleteDirectory(tmpJavaScriptDir);.

If FileUtils.deleteDirectory(tmpJavaScriptDir); will be left, I think it makes sence to replace it with FileUtils.deleteQuietly(tmpJavaScriptDir); to avoid possible errors in close() method.

And finally please order the methods in WebServer according to JCC, since getTmpJavaScriptDir() method is added before the constructor in your fix for DRILL-5735.

1. Reorganized WebServer code, with Drillbit context check
2. Renamed to getOrCreateTmJavaScriptDir() method
3. Added Unit test
  a. Checks for creation of Temp Dir for running Drillbit
  b. Checks for non-creation of failed Drillbit
  c. Check for deletion of temp directory after shutdown of Drillbit
@kkhatua
Copy link
Contributor Author

kkhatua commented Feb 26, 2019

@vvysotskyi
I've made the changes and added a unit test. Please review

* Unit Test merged into TestGracefulShutdown class * Leveraged reflections to get handle on WebServer's temp directory
* Removed unnecessary File delete calls
@kkhatua
Copy link
Contributor Author

kkhatua commented Mar 2, 2019

@vvysotskyi please review the changes. I've merged the test into Graceful Shutdown tests as that class was the closest in terms of what we are testing.

@kkhatua
Copy link
Contributor Author

kkhatua commented Mar 4, 2019

@vvysotskyi Done

@kkhatua
Copy link
Contributor Author

kkhatua commented Mar 5, 2019

Updated the PR.

@kkhatua kkhatua force-pushed the DRILL-7056 branch 2 times, most recently from 3520461 to 064cb60 Compare March 6, 2019 21:07
Copy link
Member

@vvysotskyi vvysotskyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making changes, +1

lushuifeng pushed a commit to lushuifeng/drill that referenced this pull request Jun 21, 2019
xiangt920 pushed a commit to xiangt920/drill that referenced this pull request Dec 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants