curcur commented on a change in pull request #14943: URL: https://github.com/apache/flink/pull/14943#discussion_r578909351
########## File path: flink-runtime/src/main/java/org/apache/flink/runtime/state/delegate/DelegatedStateBackend.java ########## @@ -0,0 +1,29 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.runtime.state.delegate; + +import org.apache.flink.annotation.Internal; +import org.apache.flink.runtime.state.StateBackend; + +@Internal +/** + * An interface for DelegatedStateBackend. A state backend to be delegated must implement this + * interface. + */ +public interface DelegatedStateBackend extends StateBackend {} Review comment: 1. I am not saying `unwrap` is not good, I just eel unwrap is not a concept belonging to `StateBackend `, instead it is a concept belonging to `DelegateStateBackend` 2. Currently, you can think of `getDelegatedStateBackend` in `DelegateStateBackend` a simplified version of `unwrap`. > I'm curious why didn't you add unwrap method to the StateBackend interface? 1. For the delegatee/delegated matching, I do not see how adding unwrap in `StateBackend` can be simpler than the current approach; and there are several different ways we can solve the first problem: for example, the match check can be done when loading explicit delegatee; Or we can put the matching logic in DelegateStateBackend. > If we add some new delegatee then it's impossible to distinguish for which delegatee given backend can be delegated. For example, we add a new delegating backend with extended logging. How RocksDbBackend can signal that it can NOT work with it? 2. Yes, it should. Not every state backend is delegable. When implementing a new state backend, it should register the information somewhere whether it is delegable and who can delegate it. Such logic should be put within DelegatedStateBackend, and DelegateStateBackend should use such information to double check. That's the intension to introduce Interface DelegatedStateBackend and Abstract Class DelegateStateBackend > Every new backend must be marked explicitly ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
