[GitHub] flink issue #4145: [FLINK-6938][FLINK-6939] [cep] Not store IterativeConditi...
Github user kl0u commented on the issue: https://github.com/apache/flink/pull/4145 Hi @wuchong ! Nice work! I have some changes to propose in the code itself, but before moving on with this, I have some questions about the need of the change. For the second, I have commented on the JIRA (FLINK-6939) and it would be nice to finish the discussion there first, before checking the following comments. Now for the comments on the code I would suggest the following: 1) Remove the `TransitionInfo` from the `ConditionRegistry` and replace it with the `StateTransition`, as the `TransitionInfo` is just a copy of the state transition. Also make the `ConditionRegistry`, `ConditionRegistry`. 2) Pass the `ConditionRegistry` to the `State` itself in the constructor. This will simplify all the changes of the `addStateTransition()`, `addProceed()`... as they do not need the registry as an additional argument anymore. 3) If we agree to keep the `RichIterativeFunction`, then we should probably think if we can remove the non-rich `IterativeCondition` (although this is optional). For these changes, I have a branch where I tested them a bit but of course feel free to make the code nicer, and also tell me if I did any change that does not seem reasonable to you. This branch was just a fast testing branch on my side. You can find it here: https://github.com/kl0u/flink/tree/cep-iter-pr Again thanks a lot for the work and I am looking forward to you comments! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #4145: [FLINK-6938][FLINK-6939] [cep] Not store IterativeConditi...
Github user dawidwys commented on the issue: https://github.com/apache/flink/pull/4145 @dianfu I've created a JIRA https://issues.apache.org/jira/browse/FLINK-6983 for it. Let's move the discussion there. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #4145: [FLINK-6938][FLINK-6939] [cep] Not store IterativeConditi...
Github user dianfu commented on the issue: https://github.com/apache/flink/pull/4145 @dawidwys In my mind, to achieve `translating them to newly constructed graph of states`, we only need to get the state from the NFA by the `state name` stored in `ComputationState`. Not sure if my understanding is correct? For `dynamically providing new Patterns`, do you mean dynamically changing the NFA? Another approach coming into my mind is that we can store the states in another `ValueState`(compared with the partially matched events stored) and only update it if the NFA changes. What's your thought? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #4145: [FLINK-6938][FLINK-6939] [cep] Not store IterativeConditi...
Github user dawidwys commented on the issue: https://github.com/apache/flink/pull/4145 @dianfu I agree that we should consider removing states from serialization, but I have some concerns about it: 1. `State`s are part of `ComputationState`s which describe partial matches, which means they are definitely not metadata. If we do not want to store `State`s, I think we will be forced to either drop checkpointing of partial matches or implement logic of translating them to newly constructed graph of states, which I think would be hard if possible at all. 2. One feature I would like to see in FlinkCEP library is dynamically providing new Patterns(e.g. through Side Inputs), I think in this case `State`s serializing will be crucial. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #4145: [FLINK-6938][FLINK-6939] [cep] Not store IterativeConditi...
Github user wuchong commented on the issue: https://github.com/apache/flink/pull/4145 @dianfu I agree with you. I think we can improve that in another JIRA. @dawidwys I have addressed all the comments. @kl0u It'll be great if you can have a look. Cheers, Jark --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #4145: [FLINK-6938][FLINK-6939] [cep] Not store IterativeConditi...
Github user dianfu commented on the issue: https://github.com/apache/flink/pull/4145 Nice work. Just one comment: the states in NFA is metadata, maybe we should get rid of serializing them at all? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
