-
Notifications
You must be signed in to change notification settings - Fork 13k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FLINK-20400][e2e] Migrate test_streaming_sql.sh #24776
base: master
Are you sure you want to change the base?
Conversation
5fcdbb2
to
67206b7
Compare
67206b7
to
f089be6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can invoke this with @BeforeAll
instead of doing in constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use sth like this to avoid possible leaks?
try (BufferedReader bufferedReader = new BufferedReader(new StringReader(input))) {...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes good catch, addressed
|
||
@Test | ||
void testJobManagerWithBaseImage() throws ImageBuildException { | ||
final FlinkContainersSettings settings = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need a cleanup with @AfterAll
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.sh
bash based end-to-end tests to docker based FlinkContainers framework.Brief change log
test_streaming_sql.sh
logictest_streaming_sql.sh
from e2e testsVerifying this change
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation