[FLINK-20400][e2e] Migrate test_streaming_sql.sh #24776
Conversation
morazow
changed the title
morazow
force-pushed
the
mor-stream-sql-itcase
branch
6 times, most recently
from
5fcdbb2
to
67206b7
Compare
morazow
force-pushed
the
mor-stream-sql-itcase
branch
from
67206b7
to
f089be6
Compare
There was a problem hiding this comment.
Hi @morazow thanks for the PR. I quickly went through. Overall looks good to me. I just put some comments. PTAL. Thanks!
| swapPlannerScalaWithPlannerLoader(); | ||
| } | ||
|
|
||
| private void swapPlannerLoaderWithPlannerScala() { |
There was a problem hiding this comment.
Maybe we can invoke this with @BeforeAll instead of doing in constructor?
There was a problem hiding this comment.
I think this should be fine. Here I wanted to ensure that it happens before creating flink-containers since it uses the flink folder to create job and task managers. So swap should happen before that.
But I think constructor is after the BeforeAll method, so should be fine, I will check
There was a problem hiding this comment.
Added also test that checks correct planner jar is used
| private static final String DIST_DIR = System.getProperty("distDir"); | ||
|
|
||
| @Override | ||
| protected FlinkContainers createFlinkContainers() { |
There was a problem hiding this comment.
We need to ensure that DIST_DIR is not null and is not empty?
| .collect(Collectors.toList()); | ||
| if (files.size() != 1) { | ||
| throw new IllegalStateException( | ||
| "Found multiple file pattern '" + filePattern + "', expected only one."); |
There was a problem hiding this comment.
It can also contain zero patterns no?
Maybe we can improve the error message sth like "Found " + file.size() " patterms, expected only one."?
| final BufferedReader bufferedReader = new BufferedReader(new StringReader(input)); | ||
| final List<String> lines = new ArrayList<>(); | ||
| String inputLine; | ||
| while ((inputLine = bufferedReader.readLine()) != null) { |
There was a problem hiding this comment.
Maybe use sth like this to avoid possible leaks?
try (BufferedReader bufferedReader = new BufferedReader(new StringReader(input))) {...
|
|
||
| @Test | ||
| void testJobManagerWithBaseImage() throws ImageBuildException { | ||
| final FlinkContainersSettings settings = |
There was a problem hiding this comment.
You already have settings and conf as a instance variables, maybe you can also make two of them (as instance variable) with explicit names?
There was a problem hiding this comment.
Ahh yes correct, I'd rename the local ones since they are different just for this test case
| .build()) | ||
| .build(); | ||
|
|
||
| @BeforeAll |
There was a problem hiding this comment.
do we need a cleanup with @AfterAll ?
There was a problem hiding this comment.
I think not, since the it tagged with @RegisterExtension, the FlinkContainers is stopped in the afterall callback, so setup files inside the taskmanagers will be purged with container shutdown
What is the purpose of the change
Migrate
test_streaming_sql.shbash based end-to-end tests to docker based FlinkContainers framework.Brief change log
test_streaming_sql.shlogictest_streaming_sql.shfrom e2e testsVerifying this change
Does this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation