You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/05/17 09:44:46 UTC

[GitHub] [flink] HuangXingBo commented on a diff in pull request #19743: [FLINK-27657][python] Implement remote operator state backend in PyFlink

HuangXingBo commented on code in PR #19743:
URL: https://github.com/apache/flink/pull/19743#discussion_r874597805


##########
flink-python/pyflink/datastream/functions.py:
##########
@@ -119,6 +119,40 @@ def get_aggregating_state(
         pass
 
 
+class OperatorStateStore(ABC):
+    """
+    Interface for getting operator states. Currently, only :class:`~state.BroadcastState` is
+    supported.
+    .. versionadded:: 1.16.0
+    """
+
+    @abstractmethod
+    def get_broadcast_state(self, state_descriptor: MapStateDescriptor) -> BroadcastState:
+        """
+        Fetches the :class:`~state.BroadcastState` described by :class:`~state.MapStateDescriptor`,
+        which has read/write access to the broadcast operator state.
+        """
+        pass
+
+    @abstractmethod
+    def get_read_only_broadcast_state(

Review Comment:
   I don't think `OperatorStateStore` should have this method. Whether `BroadcastState` is readonly should depend on whether it is in `process_element` or `process_broad_cast_element`
   
   



##########
flink-python/pyflink/datastream/functions.py:
##########
@@ -119,6 +119,40 @@ def get_aggregating_state(
         pass
 
 
+class OperatorStateStore(ABC):
+    """
+    Interface for getting operator states. Currently, only :class:`~state.BroadcastState` is
+    supported.
+    .. versionadded:: 1.16.0
+    """
+
+    @abstractmethod
+    def get_broadcast_state(self, state_descriptor: MapStateDescriptor) -> BroadcastState:
+        """
+        Fetches the :class:`~state.BroadcastState` described by :class:`~state.MapStateDescriptor`,
+        which has read/write access to the broadcast operator state.
+        """
+        pass
+
+    @abstractmethod
+    def get_read_only_broadcast_state(
+        self, state_descriptor: MapStateDescriptor
+    ) -> ReadOnlyBroadcastState:
+        """
+        Fetches the :class:`~state.ReadOnlyBroadcastState` described by
+        :class:`~state.MapStateDescriptor`, which only has read access to the broadcast operator
+        state.
+        """
+        pass
+
+    @abstractmethod
+    def commit(self):

Review Comment:
   I think the `commit` method is related to your underlying implementation of communication, the base class `OperatorStateStore` should not include this. 



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org