-
Notifications
You must be signed in to change notification settings - Fork 184
[FLINK-17516] [e2e] Exactly-once verification E2E #108
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
fe3bd90 to
289d11b
Compare
igalshilman
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 @tzulitai, the new test looks good to me.
I have left some comments for your consideration.
...ommon/src/main/java/org/apache/flink/statefun/e2e/common/StatefulFunctionsAppContainers.java
Outdated
Show resolved
Hide resolved
| worker.start(); | ||
| } | ||
|
|
||
| private static File temporaryCheckpointDir() throws IOException { |
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 understand the need here, but I'm wondering if creating temp directories can be avoided here?
For example can we create a separate named volume, does testcontainers supports that?
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 thought about that, but testcontainers does not support named volumes, only bind mounts. I guess it's because the docker-java API itself doesn't support named volumes, and therefore also not supported in Testcontainers which is built on top of that.
However, even with named volumes, ideally we still would want to treat them as temporary resources that should be deleted after every test run - we don't want to be persisting anything beyond the test lifecycle.
So, in that sense, technically there doesn't seem to be a difference in using named volumes v.s. bind mounting temp host directories for what we need to do here?
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.
The difference is very subtle, where the named volume (on OSX, and probably on Windows) is created on the same filesystem as the containers (in the Linux VM that runs docker engine) and hence stuff like sharing a unix domain socket should work (AFIK)
Also It seems cleaner to let docker handle the whole thing and not touching the host file system directly.
But I understand the limitation, and wouldn't want to overcomplicated this!
so good to merge!
...sts/statefun-exactly-once-e2e/src/test/resources/wrapped-messages-ingress-module/module.yaml
Outdated
Show resolved
Hide resolved
...exactly-once-e2e/src/test/java/org/apache/flink/statefun/e2e/exactlyonce/ExactlyOnceE2E.java
Outdated
Show resolved
Hide resolved
|
@igalshilman I've addressed all comments except from the one about using Docker named volumes. Please let me know what you think, and if there are no further objections, I'll merge this PR. Thank you! |
…ificationModule This closes #108.
This PR adds an E2E test that verifies exactly-once semantics with failure recovery.
It is based on #109 so that this new E2E test is also run in Travis.
Please see the class level docs of
ExactlyOnceVerificationModuleandExactlyOnceE2Eon the specifics of the app used for verification, and the verification scenario.Change log
StatefulFunctionsAppContainers. While extending that class for extra functionality required by this new E2E, it was obvious that the class is growing to big and bundling too many responsibilities (test runtime functionality, and pre-test configuration). This commit refactors the class using the builder pattern.StatefulFunctionsAppContainers.Verifying
The Travis build should pass.
In the exposed master logs, you should see that the job fails due to a lost worker, is recovered and restarts.