DoFnSignature#isStateful deprecated

2020-05-25 Thread Jan Lukavský
Hi, I have come across issue with multiple way of getting a meaningful flags for DoFns. We have  a) DoFnSignature#{usesState,usesTimers,isStateful,...}, and  b) DoFnSignatures#{usesState,usesTimers,isStateful,...} These two might not (and actually are not) aligned with each other. That can

Re: DoFnSignature#isStateful deprecated

2020-05-26 Thread Luke Cwik
I believe DoFnSignature#isStateful is remnant of a bad API name choice and was renamed to usesState. I would remove DoFnSignature#isStateful as it does not seem to be used anywhere. Does DoFnSignatures#usesValueState return true if the DoFn says it needs @RequiresTimeSortedInput because of how a D

Re: DoFnSignature#isStateful deprecated

2020-05-27 Thread Jan Lukavský
Right, this might be about a definition of what these methods really should return. Currently, the most visible issue is [1]. When a DoFn has no state or timer, but is annotated with @RequiresTimeSortedInput this annotation is silently ignored, because DoFnSignature#usesState returns false and

Re: DoFnSignature#isStateful deprecated

2020-05-29 Thread Luke Cwik
To go back to your original question. I would remove the static convenience methods in DoFnSignatures since they construct a DoFnSignature and then throw it away. This construction is pretty involved, nothing as large as an IO call but it would become noticeable if it was abused. We can already se

Re: DoFnSignature#isStateful deprecated

2020-05-29 Thread Kenneth Knowles
Agree to delete them, though for different reasons. I think this code comes from a desire to have methods that can be called on a DoFn directly. And from reviewing the code history I think they are copied in from another class. So that's why they are the way they are. Accepting a DoFnSignature woul

Re: DoFnSignature#isStateful deprecated

2020-05-31 Thread Jan Lukavský
Answers inline. On 5/29/20 5:46 PM, Luke Cwik wrote: To go back to your original question. I would remove the static convenience methods in DoFnSignatures since they construct a DoFnSignature and then throw it away. This construction is pretty involved, nothing as large as an IO call but it

Re: DoFnSignature#isStateful deprecated

2020-05-31 Thread Jan Lukavský
On 5/30/20 5:39 AM, Kenneth Knowles wrote: Agree to delete them, though for different reasons. I think this code comes from a desire to have methods that can be called on a DoFn directly. And from reviewing the code history I think they are copied in from another class. So that's why they are t

Re: DoFnSignature#isStateful deprecated

2020-06-11 Thread Jan Lukavský
Hi, I'd propose the following:  - delete all DoFnSignatures.{usesState,usesTimers,...} helpers *except* for DoFnSignatures.isStateful  - DoFnSignatures.isStateful would be equal to 'signature.usesState() || signature.usesTimers() || signature.processElement().requiresTimeSortedInput()'  -

Re: DoFnSignature#isStateful deprecated

2020-06-11 Thread Reuven Lax
On Thu, Jun 11, 2020 at 1:26 AM Jan Lukavský wrote: > Hi, > > I'd propose the following: > > - delete all DoFnSignatures.{usesState,usesTimers,...} helpers *except* > for DoFnSignatures.isStateful > Why? Actually seems like maybe the opposite is better? (remove isStateful and keep the others).

Re: DoFnSignature#isStateful deprecated

2020-06-11 Thread Jan Lukavský
On 6/11/20 5:02 PM, Reuven Lax wrote: On Thu, Jun 11, 2020 at 1:26 AM Jan Lukavský > wrote: Hi, I'd propose the following:  - delete all DoFnSignatures.{usesState,usesTimers,...} helpers *except* for DoFnSignatures.isStateful Why? Actually seems lik