Handle cases where SlaveTemplate has been removed #429
Conversation
| @@ -411,9 +416,9 @@ public boolean verifyServerHostKey(String hostname, int port, String serverHostK | |||
| } | |||
| } | |||
|
|
|||
| private String getEC2HostAddress(EC2Computer computer) throws InterruptedException { | |||
| private static String getEC2HostAddress(EC2Computer computer, SlaveTemplate template) throws InterruptedException { | |||
There was a problem hiding this comment.
Why static? I cannot see when it's needed
There was a problem hiding this comment.
I changed it to static because it doesn't need any value in the actual class itself so it should have been static from the start as its not required to be tied to any instance of the Launcher
There was a problem hiding this comment.
your words make sense, but it looks weird to me having a static private method which is not called within a static block of code. Anyway, I'm not blocking the PR because of this. It's just a matter of taste and I only wanted to know if there were other reasons. I will let the others give their opinion if applies and I'm approving the PR.
There was a problem hiding this comment.
Ahh for me I used to mostly write C++, so a static method has the benefit of not passing in an additional parameter(The this pointer). So the habit stayed with me.
| @@ -411,9 +416,9 @@ public boolean verifyServerHostKey(String hostname, int port, String serverHostK | |||
| } | |||
| } | |||
|
|
|||
| private String getEC2HostAddress(EC2Computer computer) throws InterruptedException { | |||
| private static String getEC2HostAddress(EC2Computer computer, SlaveTemplate template) throws InterruptedException { | |||
There was a problem hiding this comment.
your words make sense, but it looks weird to me having a static private method which is not called within a static block of code. Anyway, I'm not blocking the PR because of this. It's just a matter of taste and I only wanted to know if there were other reasons. I will let the others give their opinion if applies and I'm approving the PR.
Can't use IllegalState as its not caught outside and does not terminate the instance. It might also lead to a FD leak scenario(which seems to be a bug in core) |
|
If there are no objections to merging it as is, I will be merging tomorrow |
Recently we encoutered an issue where when a SlaveTemplate is removed but an old Instance that uses this slave template still exists it will throw an NPE.
This PR annotates the methods and reworks the code to handle such scenarios more gracefully.