-
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-16949] Enhance AbstractStreamOperatorTestHarness to use customized TtlTimeProvider #11624
[FLINK-16949] Enhance AbstractStreamOperatorTestHarness to use customized TtlTimeProvider #11624
Conversation
…ized TtlTimeProvider
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 6687976 (Thu Apr 02 18:53:18 UTC 2020) 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. The 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:
|
@@ -258,6 +258,10 @@ protected OperatorStateBackend operatorStateBackend( | |||
} | |||
} | |||
|
|||
protected TtlTimeProvider getTtlTimeProvider() { |
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.
Why not to inject this in the constructor of StreamTaskStateInitializerImpl
?
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 just don't want to introduce too many changes here. If we plan to inject this as a filed, I am in favor of moving TtlTimeProvider
at operator level (by changing StreamTaskStateInitializer#streamOperatorStateContext
) but not in the constructor which was FLINK-14156 fixed.
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 either approach can be refactored in future.
Using constructor is the usual way how we inject dependencies for tests. The inheritance is usually used to change more complicated internal behaviour.
You could also keep the current constructor and add another one annotated with @VisibleForTesting which could accept the custom TtlTimeProvider
field.
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.
There existed another problem why we not change the constructor of StreamTaskStateInitializerImpl
.
Current AbstractStreamOperatorTestHarness
is not created from a builder, and once a new AbstractStreamOperatorTestHarness
is created, the inner streamTaskStateInitializer
has been created with the default TtlTimeProvider
. Even we set ttl time provider to AbstractStreamOperatorTestHarness
later, the inner streamTaskStateInitializer
would not notice the changed ttl time provider unless we call AbstractStreamOperatorTestHarness#setup
to re-create the inner streamTaskStateInitializer
.
However, AbstractStreamOperatorTestHarness#setup
actually call a deprecated SetupableStreamOperator#setup
interface.
In a nutshell, unless we refactor how we build AbstractStreamOperatorTestHarness
, to make the customized ttl time provider take effect, we must call AbstractStreamOperatorTestHarness#setup
each time which might already be treated as a deprecated interface.
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.
Then I think it makes sense to consider builders to avoid adding more harness constructors.
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.
Why not AbstractStreamOperatorTestHarness
always use a mocked TtlTimeProvider
? Just like always use the TestProcessingTimeService
. Then we don't need to re-create the inner streamTaskStateInitializer
.
I also prefer the way to set processing time on the harness object AbstractStreamOperatorTestHarness#setProcessingTime
instead of on other object mockTtlTimeProvider.setCurrentTimeStamp(0L)
. We can provide a method setStateTtlTime()
on the AbstractStreamOperatorTestHarness
too.
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.
This would indeed work. This is not flexible in general but may be good enough and we already have it for processing time.
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 @wuchong 's suggestion is also a available choice. However, I think FLINK-17011 is the most clean solution to clean up these test code. Although I have to admit that PR #11676 is a bit large for reviewing.
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.
What about going with this approach first? This can be merged faster and unblock other works. We can also continue the cleanup work using new builders after that, and expose custom time provider in builders to make it more flexible.
...ava/src/test/java/org/apache/flink/streaming/util/AbstractStreamOperatorTestHarnessTest.java
Outdated
Show resolved
Hide resolved
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 the updating @Myasuka , LGTM. Just left a minor comment.
flink-runtime/src/test/java/org/apache/flink/runtime/state/ttl/MockTtlTimeProvider.java
Outdated
Show resolved
Hide resolved
…customized TtlTimeProvider This closes apache#11624
Merging... |
What is the purpose of the change
Enhance specific UTs to use customized
TtlTimeProvider
to simulate changed current time. This would introduce some changes toAbstractStreamOperatorTestHarness
and add newStreamTaskStateInitializerTestImpl
for different state backends.Brief change log
Introduce some changes to
AbstractStreamOperatorTestHarness
and add newStreamTaskStateInitializerTestImpl
for different state backends.Verifying this change
This change added tests and can be verified as follows:
AbstractStreamOperatorTestHarnessTest#testSetTtlTimeProvider
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation