Break inheritance dependency on java.lang.Process

[jdpgrailsdev]

What

• Remove the direct inheritance dependency on java.lang.Process to avoid issues caused by partial implementation

How

• Remove extension from java.lang.Process
• Add ability to create a java.lang.Process implementation from existing "process" objects

This PR is inspired by a discussion following an incident where the use of .pid() in KubePodProcess lead to failures, as the parent class (java.lang.Process) throws exceptions for certain non-abstract, public methods that are not implemented by the child class. More broadly speaking, the classes that we have identified as "processes" are not really processes in the Java sense (e.g. they are not native processes provided by an operating system). This change will break that dependency so that changes made to these classes will not be exposed to the problem outlined above (e.g. calling a method on the parent class that is not and cannot really be implemented by the child class due to the fact that it is not a true native process). This is analogous to extending java.lang.Thread instead of implementing Runnable or Callable.

Recommended reading order

1. KubePod.java
2. KubePodProcess.java
3. AsyncOrchestratorPodProcess.java
4. KubeProcessFactory.java

Tests

• All unit tests pass
• All Kube acceptance tests pass locally
• Project builds locally

[cgardens]

lgtm!

We have a parallel antipattern in our docker stack (see: DockerProcessFactory). I was surprised that this change was possible without having to touch it. Maybe that says something about code reuse about K8s and docker impls. Don't know if it is worth doing this in DockerProcessFactory too. This PR is good though and should not be blocked on that.

[davinchia]

@jdpgrailsdev just to make sure I understand, this change makes it so that we continue to use the Process interface (which we want) while restricting consumer of Airbyte process implementation from accessing Process interface internal information (which we don't want).

Is that right?

[jdpgrailsdev]

@jdpgrailsdev just to make sure I understand, this change makes it so that we continue to use the Process interface (which we want) while restricting consumer of Airbyte process implementation from accessing Process interface internal information (which we don't want).

Is that right?

@davinchia Exactly correct. We still want to represent it as a process, without the extra baggage of extending the Process class, which results in an incomplete implementation.

[jdpgrailsdev]

lgtm!

We have a parallel antipattern in our docker stack (see: DockerProcessFactory). I was surprised that this change was possible without having to touch it. Maybe that says something about code reuse about K8s and docker impls. Don't know if it is worth doing this in DockerProcessFactory too. This PR is good though and should not be blocked on that.

@cgardens I'll take a look at DockerProcessFactory. I did a scan for classes that extend java.lang.Process and didn't see any other hits than KubePodProcess, so I'll take another pass to see what I missed and open a separate PR if I find anything related to this type of change.

[jdpgrailsdev]

@cgardens I took a look at DockerProcessFactory and it is doing the correct thing already. Instead of creating a new instance of a class that we created that extends java.lang.Process, it uses java.lang.ProcessBuilder to create a new java.lang.Process instance (the correct way to create processes in Java). Therefore, I don't believe that we have the same risk with the DockerProcessFactory that we have with the KubePodProcess/KubeProcessFactory classes.