XComp commented on code in PR #18893:
URL: https://github.com/apache/flink/pull/18893#discussion_r1099965873
##########
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/api/TableITCase.scala:
##########
@@ -119,17 +118,10 @@ class TableITCase(tableEnvName: String, isStreaming:
Boolean) extends AbstractTe
assertEquals(ResultKind.SUCCESS_WITH_CONTENT, tableResult.getResultKind)
val it = tableResult.collect()
it.close()
- val jobStatus =
- try {
- Some(tableResult.getJobClient.get().getJobStatus.get())
- } catch {
- // ignore the exception,
- // because the MiniCluster maybe already been shut down when getting
job status
- case _: Throwable => None
- }
- if (jobStatus.isDefined) {
- assertNotEquals(JobStatus.RUNNING, jobStatus.get)
- }
+
+ // wait for mini cluster to shut down
+ val jobId = tableResult.getJobClient.get().getJobID
+
AbstractTestBase.MINI_CLUSTER_RESOURCE.getClusterClient.requestJobResult(jobId).get()
Review Comment:
Can you elaborate a bit on the history of this test? It looks like it was
added with FLINK-18061. IIUC, the intention of this test was to verify that
`close` cancelled the underlying job. Shouldn't we assert that? But I see the
risk of this causing an instability again because the `MyTable` contains
bounded data which might result in the job finishing successfully before
`close` is called. That would result in the test not testing the initially
intended test case. I'm curious about you view on this as I might miss
something here.
--
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]
