cameronlee314 commented on a change in pull request #1583:
URL: https://github.com/apache/samza/pull/1583#discussion_r806159097
##########
File path:
samza-core/src/main/java/org/apache/samza/runtime/ContainerLaunchUtil.java
##########
@@ -179,15 +181,26 @@ private static void run(
if (containerRunnerException != null) {
log.error("Container stopped with Exception. Exiting process now.",
containerRunnerException);
- System.exit(1);
+ exitProcess(1);
}
} catch (Throwable e) {
- log.error("Container stopped with Exception. ",
containerRunnerException);
+ log.error("Exiting the process due to {}. \nContainer runner exception:
{}", e, containerRunnerException);
Review comment:
I didn't think the logger actually prints the full stack trace if you do
it this way. I think it will just print some message, but it would be helpful
to have the full stack trace. If so, could you log them separately instead?
Maybe you could also do a null check on `containerRunnerException` for printing
it out or not.
##########
File path:
samza-core/src/main/java/org/apache/samza/runtime/ContainerLaunchUtil.java
##########
@@ -179,15 +181,26 @@ private static void run(
if (containerRunnerException != null) {
log.error("Container stopped with Exception. Exiting process now.",
containerRunnerException);
- System.exit(1);
+ exitProcess(1);
}
} catch (Throwable e) {
- log.error("Container stopped with Exception. ",
containerRunnerException);
+ log.error("Exiting the process due to {}. \nContainer runner exception:
{}", e, containerRunnerException);
+ exitProcess(1);
Review comment:
I believe `System.exit` will prevent the `finally` block from executing,
so you are slightly changing the currently functionality. That's probably ok,
since the `containerRunnerException != null` case above already does
`System.exit` and doing clean up shouldn't be absolutely necessary, but I
wanted to just point it out in case you think it should be something to handle
or leave a comment about.
##########
File path:
samza-core/src/test/java/org/apache/samza/runtime/TestContainerLaunchUtil.java
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.samza.runtime;
+
+import java.util.Optional;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import org.apache.samza.application.descriptors.ApplicationDescriptorImpl;
+import org.apache.samza.config.Config;
+import org.apache.samza.coordinator.metadatastore.CoordinatorStreamStore;
+import org.apache.samza.job.model.JobModel;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.powermock.api.mockito.PowerMockito;
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
+
+import static org.junit.Assert.*;
+import static org.mockito.Mockito.*;
+
+
+@RunWith(PowerMockRunner.class)
+@PrepareForTest(ContainerLaunchUtil.class)
+public class TestContainerLaunchUtil {
+ private static final String JOB_NAME = "test-job";
+ private static final String JOB_ID = "test-job-i001";
+ private static final String CONTAINER_ID = "test-job-container-0001";
+
+ private static final ApplicationDescriptorImpl APP_DESC =
mock(ApplicationDescriptorImpl.class);
+ private static final JobModel JOB_MODEL = mock(JobModel.class);
+ private static final Config CONFIG = mock(Config.class);
+
+ @Test
+ public void testRunWithException() throws Exception {
+ final CountDownLatch completionLatch = new CountDownLatch(1);
+ PowerMockito.mockStatic(ContainerLaunchUtil.class);
+ PowerMockito.doReturn(mock(CoordinatorStreamStore.class))
+ .when(ContainerLaunchUtil.class, "buildCoordinatorStreamStore",
eq(CONFIG), any());
+ PowerMockito.doAnswer(invocation -> {
+ completionLatch.countDown();
+ return null;
+ }).when(ContainerLaunchUtil.class, "exitProcess", eq(1));
+ PowerMockito.doCallRealMethod()
+ .when(ContainerLaunchUtil.class, "run", eq(APP_DESC), eq(JOB_NAME),
eq(JOB_ID), eq(CONTAINER_ID), any(), any(),
+ eq(JOB_MODEL), eq(CONFIG), any());
+
+ ContainerLaunchUtil.run(APP_DESC, JOB_NAME, JOB_ID, CONTAINER_ID,
Optional.empty(), Optional.empty(), JOB_MODEL,
+ CONFIG, Optional.empty());
+ assertTrue(completionLatch.await(1, TimeUnit.SECONDS));
Review comment:
Minor: Can you use `PowerMockito.verifyStatic` to track calls instead of
using your own `CountDownLatch`?
##########
File path:
samza-core/src/main/java/org/apache/samza/runtime/ContainerLaunchUtil.java
##########
@@ -179,15 +187,33 @@ private static void run(
if (containerRunnerException != null) {
log.error("Container stopped with Exception. Exiting process now.",
containerRunnerException);
- System.exit(1);
+ exitCode = 1;
}
} catch (Throwable e) {
- log.error("Container stopped with Exception. ",
containerRunnerException);
+ /*
+ * Two separate log statements are intended to print the entire stack
trace as part of the logs. Using
+ * single log statement with custom format requires explicitly fetching
stack trace and null checks which makes
+ * the code slightly hard to read in comparison with the current choice.
+ */
+ log.error("Exiting the process due to", e);
+ log.error("Container runner exception: ", containerRunnerException);
+ exitCode = 1;
} finally {
coordinatorStreamStore.close();
+ exitProcess(exitCode);
Review comment:
Do we want to be explicitly doing `System.exit(0)` for the cases where
there is no exception? The existing functionality is that this method exits
normally when there is no exception. If something calls `ContainerLaunchUtil`,
then doing `System.exit(0)` skips allowing the caller to finish.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
