jerryshao commented on code in PR #13250:
URL: https://github.com/apache/gravitino/pull/13250#discussion_r4059082731


##########
core/src/test/java/org/apache/gravitino/job/local/TestLocalJobExecutor.java:
##########
@@ -259,6 +266,236 @@ public void 
testSubmitSparkJobRejectedWhenSparkSubmitIsNotAvailable() throws IOE
     }
   }
 
+  @Test
+  public void testGetJobOutputSuccessfully() throws IOException {
+    Map<String, String> jobConf =
+        ImmutableMap.of(
+            "arg1", "value1",
+            "arg2", "success",
+            "var", "value3");
+
+    JobTemplate template =
+        JobManager.createRuntimeJobTemplate(jobTemplateEntity, jobConf, 
workingDir);
+
+    String jobId = jobExecutor.submitJob(template);
+    Awaitility.await()
+        .atMost(3, TimeUnit.MINUTES)
+        .until(() -> jobExecutor.getJobStatus(jobId) == 
JobHandle.Status.SUCCEEDED);
+
+    List<String> stdout = jobExecutor.getJobStdout(jobId, 1000, 
DEFAULT_TEST_MAX_BYTES);
+    Assertions.assertEquals(6, stdout.size());
+    Assertions.assertEquals("starting test test job", stdout.get(0));
+    Assertions.assertEquals("in common script", stdout.get(1));
+    Assertions.assertTrue(stdout.get(2).startsWith("Submitting job with 
name:"));
+    Assertions.assertEquals("value1", stdout.get(3));
+    Assertions.assertEquals("success", stdout.get(4));
+    Assertions.assertEquals("value3", stdout.get(5));
+
+    // The test script never writes to stderr.
+    Assertions.assertEquals(
+        Collections.emptyList(), jobExecutor.getJobStderr(jobId, 1000, 
DEFAULT_TEST_MAX_BYTES));
+
+    // The full output has 6 lines; only the last 3 should be returned when 
capped.
+    Assertions.assertEquals(
+        ImmutableList.of("value1", "success", "value3"),
+        jobExecutor.getJobStdout(jobId, 3, DEFAULT_TEST_MAX_BYTES));
+  }
+
+  @Test
+  public void testGetJobOutputForUnknownJobReturnsEmpty() {
+    // A job unknown to this executor - whether it never existed here, or its 
bookkeeping has
+    // expired/been lost - reports empty output rather than throwing: the job 
entity itself may
+    // still exist, and querying its output must not turn that into an error.
+    Assertions.assertEquals(
+        Collections.emptyList(),
+        jobExecutor.getJobStdout("no-such-job", 100, DEFAULT_TEST_MAX_BYTES));
+    Assertions.assertEquals(
+        Collections.emptyList(),
+        jobExecutor.getJobStderr("no-such-job", 100, DEFAULT_TEST_MAX_BYTES));
+  }
+
+  @Test
+  public void testGetJobOutputForQueuedJobReturnsEmpty() throws IOException {
+    LocalJobExecutor exec = new LocalJobExecutor();
+    exec.initialize(ImmutableMap.of(LocalJobExecutorConfigs.MAX_RUNNING_JOBS, 
"1"));
+
+    File workingDirA = 
Files.createTempDirectory("gravitino-test-local-job-executor-a").toFile();
+    File workingDirB = 
Files.createTempDirectory("gravitino-test-local-job-executor-b").toFile();
+    try {
+      Map<String, String> jobConf =
+          ImmutableMap.of(
+              "arg1", "value1",
+              "arg2", "success",
+              "var", "value3");
+
+      // Submit two jobs to a single-threaded executor - the second one stays 
QUEUED until the
+      // first (which sleeps for a few seconds) finishes.
+      JobTemplate templateA =
+          JobManager.createRuntimeJobTemplate(jobTemplateEntity, jobConf, 
workingDirA);
+      JobTemplate templateB =
+          JobManager.createRuntimeJobTemplate(jobTemplateEntity, jobConf, 
workingDirB);
+      exec.submitJob(templateA);
+      String jobIdB = exec.submitJob(templateB);
+
+      Assertions.assertEquals(JobHandle.Status.QUEUED, 
exec.getJobStatus(jobIdB));
+      Assertions.assertEquals(
+          Collections.emptyList(), exec.getJobStdout(jobIdB, 100, 
DEFAULT_TEST_MAX_BYTES));
+      Assertions.assertEquals(
+          Collections.emptyList(), exec.getJobStderr(jobIdB, 100, 
DEFAULT_TEST_MAX_BYTES));
+
+      Awaitility.await()
+          .atMost(3, TimeUnit.MINUTES)
+          .until(() -> exec.getJobStatus(jobIdB) == 
JobHandle.Status.SUCCEEDED);
+    } finally {
+      exec.close();
+      FileUtils.deleteDirectory(workingDirA);
+      FileUtils.deleteDirectory(workingDirB);
+    }
+  }
+
+  @Test
+  public void testGetJobOutputWithOversizedSingleLineIsBoundedByMaxBytes() 
throws IOException {
+    Map<String, String> jobConf =
+        ImmutableMap.of(
+            "arg1", "value1",
+            "arg2", "success",
+            "var", "value3");
+
+    JobTemplate template =
+        JobManager.createRuntimeJobTemplate(jobTemplateEntity, jobConf, 
workingDir);
+
+    String jobId = jobExecutor.submitJob(template);
+    Awaitility.await()
+        .atMost(3, TimeUnit.MINUTES)
+        .until(() -> jobExecutor.getJobStatus(jobId) == 
JobHandle.Status.SUCCEEDED);
+
+    // Overwrite the captured output with a single line far larger than the 
byte window, with no
+    // trailing newline - the pathological case a byte-bounded tail read must 
stay safe against
+    // (a naive line-oriented reverse reader can degrade badly on content 
shaped like this).
+    int maxBytes = 1024;
+    String oversizedLine = StringUtils.repeat('x', maxBytes * 4);
+    FileUtils.writeStringToFile(new File(workingDir, "output.log"), 
oversizedLine, "UTF-8");

Review Comment:
   Added both at cbdf957: 
`testGetJobOutputWithOversizedSingleLineEndingInNewlineIsTruncatedNotEmpty` and 
`testGetJobOutputWithOrdinaryLinesFollowedByOversizedFinalLine`. Both would 
have caught the bug you flagged above before this fix — they now pass against 
the corrected `readLastLines`.



-- 
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]

Reply via email to