[
https://issues.apache.org/jira/browse/FLINK-14833?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16980061#comment-16980061
]
Yu Li edited comment on FLINK-14833 at 11/22/19 10:55 AM:
----------------------------------------------------------
Personally I'm +1 on removing the hierarchy, and below are detailed reasons.
* I guess the original thought to make {{HeapSnapshotStrategy}} implementing
{{SnapshotStrategySynchronicityBehavior}} was to follow the decorator pattern
** I'm not the original author but just refactored/extracted the class out of
{{HeapKeyedStateBackend}} in FLINK-11730, so it's just a guess.
* However, there's no "decoration" on top of the
{{SnapshotStrategySynchronicityBehavior}} field.
* Checking the invocation hierarchy, there's also no dependency on using
decorator pattern, and I could not think of a possibility of future requirement
for a decorator here.
* From the javadoc of {{SnapshotStrategySynchronicityBehavior}}, a plain
composition seems to be more proper
{code}
/**
* Interface for synchronicity behavior of heap snapshot strategy.
*
* @param <K> The data type that the serializer serializes.
*/
interface SnapshotStrategySynchronicityBehavior<K> {
{code}
[~wind_ljy] would you like to submit a PR for this? We could also see whether
any different opinion from others during the PR review. Thanks.
was (Author: carp84):
Personally I'm +1 on removing the hierarchy, and below are detailed reasons.
* I guess the original thought to make {{HeapSnapshotStrategy}} implementing
{{SnapshotStrategySynchronicityBehavior}} was to follow the decorator pattern
** I'm not the original author but just refactored/extracted the class out of
{{HeapKeyedStateBackend}} in FLINK-11730, so it's just a guess.
* However, there's no "decoration" on top of the
{{SnapshotStrategySynchronicityBehavior}} field.
* Checking the invocation hierarchy, there's also no dependency on using
decorator pattern here.
* From the javadoc of {{SnapshotStrategySynchronicityBehavior}}, a plain
composition seems to be more proper
{code}
/**
* Interface for synchronicity behavior of heap snapshot strategy.
*
* @param <K> The data type that the serializer serializes.
*/
interface SnapshotStrategySynchronicityBehavior<K> {
{code}
[~wind_ljy] would you like to submit a PR for this? We could also see whether
any different opinion from others during the PR review. Thanks.
> Remove unnecessary SnapshotStrategySynchronicityBehavior interface from
> HeapSnapshotStrategy
> --------------------------------------------------------------------------------------------
>
> Key: FLINK-14833
> URL: https://issues.apache.org/jira/browse/FLINK-14833
> Project: Flink
> Issue Type: Improvement
> Components: Runtime / State Backends
> Affects Versions: 1.9.1
> Reporter: Jiayi Liao
> Priority: Major
>
> Since all methods implementing from {{SnapshotStrategySynchronicityBehavior}}
> in {{HeapSnapshotStrategy}} are executing as the same pattern below:
> {code:java}
> @Override
> public void finalizeSnapshotBeforeReturnHook(Runnable runnable) {
>
> snapshotStrategySynchronicityTrait.finalizeSnapshotBeforeReturnHook(runnable);
> }
> @Override
> public boolean isAsynchronous() {
> return snapshotStrategySynchronicityTrait.isAsynchronous();
> }
> @Override
> public <N, V> StateTable<K, N, V> newStateTable(
> InternalKeyContext<K> keyContext,
> RegisteredKeyValueStateBackendMetaInfo<N, V> newMetaInfo,
> TypeSerializer<K> keySerializer) {
> return snapshotStrategySynchronicityTrait.newStateTable(keyContext,
> newMetaInfo, keySerializer);
> }
> {code}
> It looks like implementing the {{SnapshotStrategySynchronicityBehavior}}
> interface is not necessary for {{HeapSnapshotStrategy}} and we can just
> remove it and the related {{@Override}} annotations. And
> {{HeapSnapshotStrategy}} doesn't match the java docs in
> {{SnapshotStrategySynchronicityBehavior}} also.
>
> And please correct me if there is a reason here.
>
> cc [~liyu]
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
