[GitHub] flink issue #5295: [FLINK-8384] [streaming] Session Window Assigner with Dyn...

Github user dyanarose commented on the issue: https://github.com/apache/flink/pull/5295 I can see it's gone through Travis and is now in master, so closing as requested ---

[GitHub] flink issue #5295: [FLINK-8384] [streaming] Session Window Assigner with Dyn...

Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/5295 Thanks a lot for working on this and iterating so quickly! 👍 I merged this but could you please close the PR if it doesn't close automatically? ---

[GitHub] flink issue #5295: [FLINK-8384] [streaming] Session Window Assigner with Dyn...

Github user dyanarose commented on the issue: https://github.com/apache/flink/pull/5295 the change to return Time has been backed out, so extract returns a long again. PublicEvolving annotations have been added to the new classes and methods. ---

[GitHub] flink issue #5295: [FLINK-8384] [streaming] Session Window Assigner with Dyn...

Github user dyanarose commented on the issue: https://github.com/apache/flink/pull/5295 erf, I see what you mean, as well as the creation of all those Time objects. ---

[GitHub] flink issue #5295: [FLINK-8384] [streaming] Session Window Assigner with Dyn...

Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/5295 I get that logic, but the existing `TimestampAssigner` also returns a `long` and if we return `Time` we always have to wrap/unwrap that long. What do you think? ---

[GitHub] flink issue #5295: [FLINK-8384] [streaming] Session Window Assigner with Dyn...

Github user dyanarose commented on the issue: https://github.com/apache/flink/pull/5295 I like long myself, but I think that's only because I'm quite used to working in milliseconds. As the existing static Session Windows take Time as the gap, I think it made sense to have the

[GitHub] flink issue #5295: [FLINK-8384] [streaming] Session Window Assigner with Dyn...

Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/5295 I think the changes are good! Thanks for working on this. 👍 As a final change before merging, I would annotate the new classes/methods as `@PublicEvolving`, would you be ok with that?

[GitHub] flink issue #5295: [FLINK-8384] [streaming] Session Window Assigner with Dyn...

Github user dyanarose commented on the issue: https://github.com/apache/flink/pull/5295 the ci fail looks to be a known flaky test: FlinkKafkaProducer011ITCase.testScaleDownBeforeFirstCheckpoint ---

[GitHub] flink issue #5295: [FLINK-8384] [streaming] Session Window Assigner with Dyn...

Github user dyanarose commented on the issue: https://github.com/apache/flink/pull/5295 Ah, I hadn't thought to keep both in place. So unless the Dynamic SessionWindow classes had withDynamicGap made package private, you would then be able to instantiate them from two different

[GitHub] flink issue #5295: [FLINK-8384] [streaming] Session Window Assigner with Dyn...

Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/5295 Yes, that was me. 😅 It was just a quick idea, and it would work because both `withGap()` and `withGapExtractor()` are static so the latter could have `T` on the method signature and return a

[GitHub] flink issue #5295: [FLINK-8384] [streaming] Session Window Assigner with Dyn...

Github user dyanarose commented on the issue: https://github.com/apache/flink/pull/5295 I'm not the biggest fan of unchecked casts, but testing this in our POC environment casting the existing EventTimeTrigger to a typed Trigger is working. So if the unchecked cast is

[GitHub] flink issue #5295: [FLINK-8384] [streaming] Session Window Assigner with Dyn...

Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/5295 I like the functionality of this a lot! However, I don't like that we change the signature of existing triggers or that we introduce new triggers that duplicate existing code. As an

[GitHub] flink issue #5295: [FLINK-8384] [streaming] Session Window Assigner with Dyn...

Github user sunjincheng121 commented on the issue: https://github.com/apache/flink/pull/5295 It's `NoResourceAvailableException` error, not sure, but we can try to rebuild it. ---

[GitHub] flink issue #5295: [FLINK-8384] [streaming] Session Window Assigner with Dyn...

Github user dyanarose commented on the issue: https://github.com/apache/flink/pull/5295 looks like the build failed on: org.apache.flink.test.streaming.runtime.StreamTaskTimerITCase testOperatorChainedToSource I can't see why this change would cause that to fail, after