Mailing-List: contact issues-help@flink.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@flink.apache.org
Date: Thu, 16 Mar 2017 20:07:41 +0000 (UTC)
From: "ASF GitHub Bot (JIRA)" <jira@apache.org>
To: issues@flink.apache.org
Message-ID: <JIRA.13049868.1489120436000.41755.1489694861700@Atlassian.JIRA>
In-Reply-To: <JIRA.13049868.1489120436000@Atlassian.JIRA>
References: <JIRA.13049868.1489120436000@Atlassian.JIRA> <JIRA.13049868.1489120436464@jira-lw-us.apache.org>
Subject: [jira] [Commented] (FLINK-6018) Properly initialise StateDescriptor
 in AbstractStateBackend.getPartitionedState()
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
archived-at: Thu, 16 Mar 2017 20:07:46 -0000


    [ https://issues.apache.org/jira/browse/FLINK-6018?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15928775#comment-15928775 ] 

ASF GitHub Bot commented on FLINK-6018:
---------------------------------------

Github user StephanEwen commented on the issue:

    https://github.com/apache/flink/pull/3534
  
    This change is probably uncritical:
      - It seems this code path was never executed (by chance) because the `RuntimeContext` pre-initializes state descriptors, and all the operators directly supply serializers to the descriptors.
      - As @aljoscha mentioned, in the current case or the KryoSerializer, adding more configuration is not a problem.
    
    Okay, +1, this seems safe.


> Properly initialise StateDescriptor in AbstractStateBackend.getPartitionedState()
> ---------------------------------------------------------------------------------
>
>                 Key: FLINK-6018
>                 URL: https://issues.apache.org/jira/browse/FLINK-6018
>             Project: Flink
>          Issue Type: Improvement
>          Components: DataStream API, State Backends, Checkpointing
>            Reporter: sunjincheng
>            Assignee: sunjincheng
>             Fix For: 1.3.0
>
>
> The code snippet currently in the `AbstractKeyedStateBackend # getPartitionedState` method, as follows:
> {code}
> line 352: // TODO: This is wrong, it should throw an exception that the initialization has not properly happened
> line 353: if (!stateDescriptor.isSerializerInitialized()) {
> line 354:        stateDescriptor.initializeSerializerUnlessSet(new ExecutionConfig());
> line 354 }
> {code}
> Method `isSerializerInitialized`:
> {code}
> public boolean isSerializerInitialized() {
> 		return serializer != null;
> 	}
> {code}
> Method `initializeSerializerUnlessSet`:
> {code}
> public void initializeSerializerUnlessSet(ExecutionConfig executionConfig) {
> 		if (serializer == null) { 
> 			if (typeInfo != null) {
> 				serializer = typeInfo.createSerializer(executionConfig);
> 			} else {
> 				throw new IllegalStateException(
> 						"Cannot initialize serializer after TypeInformation was dropped during serialization");
> 			}
> 		}
> 	}
> {code}
> that is, in the `initializeSerializerUnlessSet` method, The `serializer` has been checked by `serializer == null`.So I hope this code has a little improvement to the following:
> approach 1: 
> According to the `TODO` information  we throw an exception
> {code}
> if (!stateDescriptor.isSerializerInitialized()) {
> 			throw new IllegalStateException("The serializer of the descriptor has not been initialized!"); 
> }
> {code}
> approach 2:
> Try to initialize and remove `if (!stateDescriptor.isSerializerInitialized()) {` logic.
> {code}
> stateDescriptor.initializeSerializerUnlessSet(new ExecutionConfig());
> {code}
> Meanwhile, If we use the approach 2, I suggest that `AbstractKeyedStateBackend` add a `private final ExecutionConfig executionConfig` property. then we can change the code like this:
> {code}
> stateDescriptor.initializeSerializerUnlessSet(executionConfig);
> {code}
> Are the above suggestions reasonable for you? 
> Welcome anybody's feedback and corrections.


--
This message was sent by Atlassian JIRA
(v6.3.15#6346)