-
Notifications
You must be signed in to change notification settings - Fork 13.9k
[FLINK-11012] Introduce abstract superclass for filesystem IT cases #7384
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
Conversation
|
@StefanRRichter Please help to review this PR, thanks. |
|
@StefanRRichter Please help to review this PR, thanks :) |
|
@StefanRRichter Would you pls review this PR? It has been pending for half a month, thanks. |
StefanRRichter
left a comment
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.
Thanks for your contribution, I think it looks very good. I will partition this into three commits, before merging (1 code fix, 1 doc fix, the test rewrites). 👍
|
|
||
| if (scheme.startsWith("s3") || scheme.startsWith("emr")) { | ||
| // the Amazon S3 storage | ||
| if (scheme.startsWith("s3") || scheme.startsWith("emr") || scheme.startsWith("oss")) { |
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 this change should go into s separate commit, either with a different jira or as hotfix.
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.
Sounds good, I think hotfix will be better.
|
Thanks @StefanRRichter |
|
Ok, please upload the result and I will merge after that. |
|
@StefanRRichter Error message: However, when I enter flink-systems/flink-s3-fs-presto and ran the same command, it's OK. I found flink master branch also has this issue. Do you think I should fire another issue fix this first, or am I missing sth? |
|
Test results from this PR:
[INFO] ------------------------------------------------------- |
|
LGTM 👍 Merging. |
|
@StefanRRichter Thanks, but what about the issue I mentioned? Flink master branch tests fail if I execute command in flink-s3-fs-presto's parent directory. |
|
But you said this was the same in the master, so it is would not be a regression from this PR and can be fixed independently. |
|
|
Thanks! |
…cases. This closes apache#7384.
What is the purpose of the change
IT cases for filesystems are somewhat similar as they are using write/read cycles and querying meta data like directory listing. We could avoid code duplication now and for the future by introducing an abstract superclass for those it cases that outlines the tests.
Brief change log
Verifying this change
This change does not change the tests logic, so they should be verified as before
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (yes / no)Documentation