-
Notifications
You must be signed in to change notification settings - Fork 37
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
Handling FileNotFound exception in AutoTuner #1065
Conversation
…uite Signed-off-by: Sayed Bilal Bari <[email protected]>
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.
Thanks Bilal. Added a few comments
case _: FileNotFoundException => | ||
logWarning("No yaml found for input path") | ||
None | ||
case e: Exception => throw e |
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.
No need for this case exception
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.
Removed
@@ -245,7 +245,6 @@ class AutoTunerSuite extends FunSuite with BeforeAndAfterEach with Logging { | |||
| path to the Spark RAPIDS plugin jar. | |||
| If the Spark RAPIDS jar is being bundled with your Spark | |||
| distribution, this step is not needed. | |||
|- java.io.FileNotFoundException: File non-existing.yaml does not exist |
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.
It is still useful to have a comment that the input path was not 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.
Updated
Signed-off-by: Sayed Bilal Bari <[email protected]>
Signed-off-by: Sayed Bilal Bari <[email protected]>
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.
Thanks @bilalbari . Just a small comment to update
val autoT = new AutoTuner(clusterPropsOpt.getOrElse(new ClusterProperties()), | ||
singleAppProvider, platform, driverInfoProvider) | ||
if(clusterPropsOpt.isEmpty) { | ||
autoT.appendComment(s"workerInfo file not found at $workerInfoFilePath. " + |
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.
Add a comment here to explain why we are making this check.
This makes the code more readable
// In case the workerInfo input path was incorrect, the AutoTuner needs to add a comment that the recommendations were generated by assuming default worker properties.
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.
Thanks @amahussein . This has been updated.
Signed-off-by: Sayed Bilal Bari <[email protected]>
Signed-off-by: Sayed Bilal Bari <[email protected]>
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.
Thanks @bilalbari
Lets wait for @tgravescs feedback on the AutoTuner's change
@@ -1215,7 +1216,15 @@ object AutoTuner extends Logging { | |||
val reader = new BufferedReader(new InputStreamReader(fsIs)) | |||
val fileContent = Stream.continually(reader.readLine()).takeWhile(_ != null).mkString("\n") | |||
loadClusterPropertiesFromContent(fileContent) | |||
} finally { | |||
} | |||
catch { |
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.
nit, these should go on previous line
" } catch {"
"} finally {"
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.
Updated
catch { | ||
// In case of missing file for cluster properties, default properties are used. | ||
// Hence, catching and logging as a warning | ||
case _: FileNotFoundException => |
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'm assuming there is a chance something else bad happens here, maybe a misformatted file or issue getting the file FileSystem.get returns IOException for instance. its probably safer to catch nonFatal errors here and log and return None
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.
Right! in this method we can capture IOException
which is more generic.
The caller method catches the NoFatal(e) to cover other sort of exceptions that we are not aware of
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.
Thanks @tgravescs @amahussein . This has been corrected
driverInfoProvider) | ||
val autoT = new AutoTuner(clusterPropsOpt.getOrElse(new ClusterProperties()), | ||
singleAppProvider, platform, driverInfoProvider) | ||
if(clusterPropsOpt.isEmpty) { |
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.
nit add a space after if
"if (cluster"
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.
Updated
Signed-off-by: Sayed Bilal Bari <[email protected]>
Signed-off-by: Sayed Bilal Bari <[email protected]>
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.
Thanks @bilalbari
LGTME
Fixes #978
This fix handles the AutoTuner exception when an input file passed as workerInfo arguments does not exist.
Changes -