[GitHub] [storm] bipinprasad commented on a change in pull request #3319: [STORM-3683] Check if JVM options used for launching worker are valid.

[GitBox <gi...@apache.org>]

bipinprasad commented on a change in pull request #3319:
URL: https://github.com/apache/storm/pull/3319#discussion_r466577874



##########
File path: storm-server/src/main/java/org/apache/storm/daemon/supervisor/BasicContainer.java
##########
@@ -568,14 +570,7 @@ private int getMemOffHeap(WorkerResources resources) {
     }
 
     protected String javaCmd(String cmd) {

Review comment:
       When I took this out, a bunch of test classes started failing. They were using a Mock class to override this class method to return just "java". And in the test, they check the returned array. In order to avoid changing a whole bunch of other classes, I left this signature unchanged. But this definitely looks ugly and the tests should be fixed.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org