> On April 22, 2016, 1:09 a.m., Jagadish Venkatraman wrote: > > samza-core/src/main/java/org/apache/samza/job/util/ProcessKiller.java, line > > 27 > > <https://reviews.apache.org/r/46546/diff/1/?file=1356097#file1356097line27> > > > > nit: > > What's the purpose of this class? > > > > 1.If this is a utility class (this is what I think it is), How about > > naming it as ProcessUtil or even a more general Utils class with a > > killForcibly (process) method. > > > > We can explicitly provide a private constructor in that case to ensure > > that we prevent instantiation of ProcessUtil. > > > > > > 2. If this is not a utility class, then, maybe an instance of a > > ProcessKiller can be instantiated with a Process object in its constructor. > > (It can then expose a kill method that will kill its process)?
I cannot, in good conscience, name any class "util". :-) In my experience, utility classes are the swiss army knives of software. They become a collection of misc tools that arise because it wasnt clear where they belonged. I much prefer calling things what they are. This is a Process killer, it kills instances of java.lang.Process. Further, rolling this method in with some other class just helps hide it. I want it to stick out like a sore thumb so we delete it as soon as we drop Java 7 support. I'm open to #2, but I don't see the value. Why instantiate a stateless Object in order to kill a Process? If this were Objective C, I would extend Process with a Category directly, avoiding the need for a separate class, but alas, it is Java and we must embrace some level of verbosity. :-) > On April 22, 2016, 1:09 a.m., Jagadish Venkatraman wrote: > > samza-core/src/main/java/org/apache/samza/job/util/ProcessKiller.java, line > > 31 > > <https://reviews.apache.org/r/46546/diff/1/?file=1356097#file1356097line31> > > > > nit: It was my understanding that destroy() was implementation > > dependent (and continues to be) in java (7 and 8). > > > > destroyForcibly() terminates a process with a much higher degree of > > success. It is implementation dependent, because users can subclass Process, but the windows and unix implementations of Process honor the forcible kills. Further details in the next answer. > On April 22, 2016, 1:09 a.m., Jagadish Venkatraman wrote: > > samza-core/src/main/java/org/apache/samza/job/util/ProcessKiller.java, line > > 33 > > <https://reviews.apache.org/r/46546/diff/1/?file=1356097#file1356097line33> > > > > nit: I'm not sure the existing contract of destroy was changed. (AFAIK, > > they added a new method in java 8).. Maybe, I'm missing something. Could > > you please provide me to a link? Thought I had my bases covered with the references in the description, lol. Restating the relevant parts here so it's clear what I was referring to. Java 7 Doc: "The subprocess represented by this Process object is *forcibly* terminated." Java 8 Doc: "Whether the subprocess represented by this Process object is forcibly terminated or not is implementation dependent." 2 further statements to support my case: 1. If you run check_all repeatedly, this test only fails with the Java 8 iterations, not the Java 7 iterations 2. If you dig into the source, you'll see that it has in fact changed, all the way down to the native method that actually kills the process. - Jake ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46546/#review130002 ----------------------------------------------------------- On April 22, 2016, 12:08 a.m., Jake Maes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46546/ > ----------------------------------------------------------- > > (Updated April 22, 2016, 12:08 a.m.) > > > Review request for samza, Navina Ramesh, Jagadish Venkatraman, Xinyu Liu, and > Yi Pan (Data Infrastructure). > > > Bugs: SAMZA-940 > https://issues.apache.org/jira/browse/SAMZA-940 > > > Repository: samza > > > Description > ------- > > SAMZA-940 TestProcessJob.testProcessJobKillShouldWork fails occasionally > > It looks like this has been a problem since Java 8. Java 8 changed the > meaning of Process.destroy(). It used to forcibly destroy the process, but > now it is a "normal termination" and destroyForcibly() is the method that > force-kills the process. > > Since destroyForcibly() was only introduced in Java 8, the best solution I > found was to use reflection to call that method if it exists. > > References: > https://docs.oracle.com/javase/7/docs/api/java/lang/Process.html#destroy() > https://docs.oracle.com/javase/8/docs/api/java/lang/Process.html#destroy-- > > Java 9 docs are a bit clearer > http://download.java.net/jdk9/docs/api/java/lang/Process.html#destroy-- > > > Diffs > ----- > > samza-core/src/main/java/org/apache/samza/job/util/ProcessKiller.java > PRE-CREATION > samza-core/src/main/scala/org/apache/samza/job/local/ProcessJob.scala > 66618165d27aa916238cc86b27631c5db3435c6a > > Diff: https://reviews.apache.org/r/46546/diff/ > > > Testing > ------- > > Ran the test a number of times and haven't seen the assertion fail. > > > Thanks, > > Jake Maes > >
