[GitHub] rdhabalia commented on a change in pull request #2214: Add tls support to authenticate client to access function admin-api

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

rdhabalia commented on a change in pull request #2214: Add tls support to authenticate client to access function admin-api
URL: https://github.com/apache/incubator-pulsar/pull/2214#discussion_r207388606
 
 

 ##########
 File path: pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/WorkerServer.java
 ##########
 @@ -96,7 +136,6 @@ public void run() {
             log.error("ex: {}", ex, ex);
             final String message = getErrorMessage(server, this.workerConfig.getWorkerPort(), ex);
             log.error(message);
-            System.exit(1);
 
 Review comment:
   @sijie integration test is failing because I have removed it in this PR. in order to test tls, we need to start worker-server and while stopping it in unit-test, it was throwing interruption exception on the thread which was causing system.exit in junit process. 
   and it's also bad practice to do system.exit on exception. 
   However, this change was causing integration test failure.
   to fix it, we can either keep system.exit for now and disable tls-unit test or we have to fix integration test.
   I wanted to refactor this method lil bit to fix it but we merge it before.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services