search

microsoft / durabletask-java

Java SDK for Durable Functions and the Durable Task Framework
MIT License
14 stars 7 forks source link

Should we have timeout paramter for `PurgeInstanceCriteria`? #189

Closed kaibocai closed 11 months ago

kaibocai commented 11 months ago

I noticed that the PurgeInstanceCriteria class has a timeout field https://github.com/microsoft/durabletask-java/blob/b3e2d3ab9dfc07495f830d90b00d7fac35009ea0/client/src/main/java/com/microsoft/durabletask/PurgeInstanceCriteria.java#L19 which is used to timeout the purgeInstance Grpc call.

This timeout field is not found in durabletask-dotnet. Also giving a timeout for the purging instance call seems meaningless, even if it's timeout, eventually, the instance will be purged as all this happens at the storage backend.

I think we should remove this timeout parameter. @cgillum, @DeepanshuA

cgillum commented 11 months ago

Also giving a timeout for the purging instance call seems meaningless, even if it's timeout, eventually, the instance will be purged as all this happens at the storage backend.

A timeout accomplishes two things:

  1. It allows the client call to be unblocked if something is taking too long (this can be important even if the server operation cannot be canceled).
  2. It allows the server to cancel what it's doing - which I think is in conflict with what you're suggesting.

Depending on the implementation, a client-specified timeout can still cause the server-side operation to be canceled. For example, in .NET, this can be done by using the cancellation token provided in the gRPC server-side context to any downstream purge operations.

Let me know if I'm misunderstanding.

kaibocai commented 11 months ago

Make sense thank you!