[FLINK-14951][tests] Harden the thread safety of State TTL backend tests #10348
Conversation
|
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 54ecee6 (Thu Nov 28 13:48:06 UTC 2019) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. DetailsThe Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
CI report:
Bot commandsThe @flinkbot bot supports the following commands:
|
| return lastReturnedProcessingTime; | ||
| } | ||
| return getCurrentTimestamp(); | ||
| return action.apply(getCurrentTimestamp()); |
There was a problem hiding this comment.
I think we still need to freeze and unfreeze time before and after this call asserting timestampAfterUpdate == timestampBeforeUpdate but now in a thread safe manner. Otherwise, this class does nothing and we again will have the problems with time discrepancies during verification which was the reason why we introduced the time freezing before. Looks like we do not really need to change the existing code too much, only introduce doWithFrozenTime and freeze/unfreeze methods can be private now w/o locking internally.
Also, doWithFrozenTime can be annotated with @GuardedBy("lock").
Sorry, I guess my comment on Jira did not have the full code.
KarmaGYZ
force-pushed
the
FLINK-14951
branch
from
54ecee6 to
3e89b2b
Compare
|
Thanks for the review @azagrebin . Addressed. |
|
@flinkbot run travis |
azagrebin
force-pushed
the
FLINK-14951
branch
from
3e89b2b to
1311090
Compare
|
|
||
| @GuardedBy("lock") | ||
| static long freeze() { | ||
| static <T, E extends Throwable> T doWithFrozenTime(FunctionWithException<Long, T, E> action) throws E { |
There was a problem hiding this comment.
I moved this method up because in our code style we try to adhere to the following method order in the class:
static fields
non-static fields
constructor1() { constructor2() }
constructor2() { }
// non static methods:
method1() { method2() }
method2() { method3() }
method3() { }
// static methods:
staticMethod1() { staticMethod2(); staticMethod3(); }
staticMethod2() { }
staticMethod3() { }
inner classes
embedded static classes
...
There was a problem hiding this comment.
Thanks for the explanation! I'll follow this style guide from now on.
azagrebin
force-pushed
the
FLINK-14951
branch
2 times, most recently
from
98b5328 to
c6eeebb
Compare
| return lastReturnedProcessingTime; | ||
| } | ||
| final long timestampBeforeUpdate = freeze(); | ||
| T result = action.apply(timestampBeforeUpdate); |
There was a problem hiding this comment.
I fixed one more thing here. We have to use the frozen time timestampBeforeUpdate because if we use getCurrentTimestamp it will change the frozen time and fail checkState later.
azagrebin
force-pushed
the
FLINK-14951
branch
from
c6eeebb to
5906e87
Compare
|
merged into master by 2c11291 |
4 participants
What is the purpose of the change
The State TTL RocksDb/file backend tests now contains thread safety issue during the call of MonotonicTTLTimeProvider. This PR harden the thread safety of that class.
Brief change log
Replace the freeze/unfreeze function with
doWithFrozenTime, which run user defined function in synchronize state.Verifying this change
This change added tests and can be verified as follows:
taskmanager.numberOfTaskSlotstoPARALLELISMand run State TTL RocksDb/file backend end to end test.Does this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation
cc @azagrebin