[GitHub] flink issue #6205: [FLINK-9642]Reduce the count to deal with state during a ...
Github user dawidwys commented on the issue: https://github.com/apache/flink/pull/6205 Hi @Aitozi , Thanks for the work here. I think we could improve a bit separation of concerns in the SharedBuffer. I am a bit afraid this class will become too complex once again. How about we split the SharedBuffer into two layers: caching one(SharedBuffer) and accessing buffer e.g. `SharedBufferAccessor`. We could make the accessor `AutoCloseable`. I think it would give more explicit information about the necessity to flush the buffer. We could move to `SharedBufferAccessor` methods like: - registerEvent - put - extractPatterns - materializeMatch - materializeMatch - lockNode - releaseNode - removeNode - lockEvent - releaseEvent In the `SharedBuffer` we would leave only the caching package-protected methods. We would also add a method there to get the `SharedBufferAccessor` that would `flush` the changes on `close`. This way we would have a cleaner separation, plus we would make the flushing more intuitive. What do you think? ---
[GitHub] flink issue #6205: [FLINK-9642]Reduce the count to deal with state during a ...
Github user dawidwys commented on the issue: https://github.com/apache/flink/pull/6205 Hi @Aitozi , Thanks for the work here. I think we could improve a bit separation of concerns in the SharedBuffer. I am a bit afraid this class will become too complex once again. How about we split the SharedBuffer into two layers: caching one(SharedBuffer) and accessing buffer e.g. `SharedBufferAccessor`. We could make the accessor `AutoCloseable`. I think it would give more explicit information about the necessity to flush the buffer. We could move to `SharedBufferAccessor` methods like: - registerEvent - put - extractPatterns - materializeMatch - materializeMatch - lockNode - releaseNode - removeNode - lockEvent - releaseEvent In the `SharedBuffer` we would leave only the caching package-protected methods. We would also add a method there to get the `SharedBufferAccessor` that would `flush` the changes on `close`. This way we would have a cleaner separation, plus we would make the flushing more intuitive. What do you think? ---
[GitHub] flink issue #6205: [FLINK-9642]Reduce the count to deal with state during a ...
Github user Aitozi commented on the issue: https://github.com/apache/flink/pull/6205 Resolved the conflicts, please help review when you free @dawidwys . ---
[GitHub] flink issue #6205: [FLINK-9642]Reduce the count to deal with state during a ...
Github user zhangminglei commented on the issue: https://github.com/apache/flink/pull/6205 Hi, @Aitozi I think it has conflicts now. ---
[GitHub] flink issue #6205: [FLINK-9642]Reduce the count to deal with state during a ...
Github user Aitozi commented on the issue: https://github.com/apache/flink/pull/6205 Could you take a look at this PR @dawidwys ? ---
[GitHub] flink issue #6205: [FLINK-9642]Reduce the count to deal with state during a ...
Github user Aitozi commented on the issue: https://github.com/apache/flink/pull/6205 Is this look ok now? ping @sihuazhou @dawidwys ---
[GitHub] flink issue #6205: [FLINK-9642]Reduce the count to deal with state during a ...
Github user Aitozi commented on the issue: https://github.com/apache/flink/pull/6205 fixed the error that the access to state increased in `NFAStateAccessTest` by add the `isEmpty` judgment before update the state. ---
[GitHub] flink issue #6205: [FLINK-9642]Reduce the count to deal with state during a ...
Github user Aitozi commented on the issue: https://github.com/apache/flink/pull/6205 Using the `entries#putAll` in `flushCache` lead to the count in `NFAStateAccessTest` increased, I will check it locally , this travis will fail. ---
[GitHub] flink issue #6205: [FLINK-9642]Reduce the count to deal with state during a ...
Github user Aitozi commented on the issue: https://github.com/apache/flink/pull/6205 The travis error this time seems unrelated. ---
[GitHub] flink issue #6205: [FLINK-9642]Reduce the count to deal with state during a ...
Github user Aitozi commented on the issue: https://github.com/apache/flink/pull/6205 Hi @zhangminglei , thanks for your review. I only check the SharedBufferTest locally before, the error in travis means the num of state access (read and write) is less than before which is the purpose of this pr, and I fix the error. cc @dawidwys ---
[GitHub] flink issue #6205: [FLINK-9642]Reduce the count to deal with state during a ...
Github user zhangminglei commented on the issue: https://github.com/apache/flink/pull/6205 Hi, @Aitozi Could you please take a look on the travis error ``` Tests run: 2, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 0.014 sec <<< FAILURE! - in org.apache.flink.cep.nfa.NFAStateAccessTest testIterativeWithABACPattern(org.apache.flink.cep.nfa.NFAStateAccessTest) Time elapsed: 0.009 sec <<< FAILURE! java.lang.AssertionError: expected:<20> but was:<8> at org.junit.Assert.fail(Assert.java:88) at org.junit.Assert.failNotEquals(Assert.java:834) at org.junit.Assert.assertEquals(Assert.java:645) at org.junit.Assert.assertEquals(Assert.java:631) at org.apache.flink.cep.nfa.NFAStateAccessTest.testIterativeWithABACPattern(NFAStateAccessTest.java:193) testComplexBranchingAfterZeroOrMore(org.apache.flink.cep.nfa.NFAStateAccessTest) Time elapsed: 0.005 sec <<< FAILURE! java.lang.AssertionError: expected:<5> but was:<2> at org.junit.Assert.fail(Assert.java:88) at org.junit.Assert.failNotEquals(Assert.java:834) at org.junit.Assert.assertEquals(Assert.java:645) at org.junit.Assert.assertEquals(Assert.java:631) at org.apache.flink.cep.nfa.NFAStateAccessTest.testComplexBranchingAfterZeroOrMore(NFAStateAccessTest.java:112) Running org.apache.flink.cep.nfa.compiler.NFACompilerTest Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.019 sec - in org.apache.flink.cep.nfa.compiler.NFACompilerTest Running org.apache.flink.cep.nfa.DeweyNumberTest Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.01 sec - in org.apache.flink.cep.nfa.DeweyNumberTest Running org.apache.flink.cep.nfa.sharedbuffer.SharedBufferTest Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.004 sec - in org.apache.flink.cep.nfa.sharedbuffer.SharedBufferTest Running org.apache.flink.cep.pattern.PatternTest Tests run: 22, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.033 sec - in org.apache.flink.cep.pattern.PatternTest Results : Failed tests: NFAStateAccessTest.testComplexBranchingAfterZeroOrMore:112 expected:<5> but was:<2> NFAStateAccessTest.testIterativeWithABACPattern:193 expected:<20> but was:<8> Tests run: 80, Failures: 2, Errors: 0, Skipped: 12 13:21:32.852 [INFO] 13:21:32.852 [INFO] Reactor Summary: 13:21:32.852 [INFO] 13:21:32.852 [INFO] flink-queryable-state-client-java .. SUCCESS [ 4.830 s] 13:21:32.852 [INFO] flink-cep .. FAILURE [ 5.861 s] 13:21:32.852 [INFO] flink-table SKIPPED 13:21:32.852 [INFO] flink-queryable-state-runtime .. SKIPPED 13:21:32.852 [INFO] flink-gelly SKIPPED 13:21:32.852 [INFO] flink-gelly-scala .. SKIPPED 13:21:32.852 [INFO] flink-gelly-examples ... SKIPPED 13:21:32.852 [INFO] flink-python ... SKIPPED 13:21:32.852 [INFO] flink-ml ... SKIPPED 13:21:32.852 [INFO] flink-cep-scala SKIPPED ``` ---
