search

conductor-sdk / conductor-csharp

The conductor-csharp repository provides the client SDKs to build task workers in C#
Apache License 2.0
41 stars 16 forks source link

Cleaned up internal classes #37

Closed gardusig closed 1 year ago

madsph commented 1 year ago

Hi. I was in need of this feature in my project, so I created something similar in a local branch. Feel free to use any of it if you want to.

batch-polling-patch.txt

Disclaimer: I was not sure where you were headed with the rest client, so I simply copy/pasted the old poll impl (including Obsolete marking) and made the change.

Thank you for your great work

gardusig commented 1 year ago

@madsph Awesome! That will be surely handy 😄

Also, what is your opinion about the current design using System.Threading.Tasks.Task<>? Our other SDKs have a slightly different strategy, kind of a custom thread pool for each worker.

We'll work to make a release soon, stay tuned 😎

madsph commented 1 year ago

@gardusig Great thanks - will be looking out for that :-)

Regarding Threads vs Tasks. I haven't given that much thought, so this is just what comes out top of my head. I am a lazy bastard :-) so I would go a long way to avoid messing around with my own thread handling, even though I see how you could make a case for threads being "the right" way of implementing parallel workers. In my project, Tasks will suffice, but I realize that that may not be the case for all projects. But I guess you could just run multiple instances of the service hosting the workers, if different threads are truly needed, and then keep the code for this project as simple as possible. Simple would in my view be using Tasks.

gardusig commented 1 year ago

Hey @madsph, I will let things move smoothly by shortening this PR to a few cleanup of the internal classes. Also agree with your answer :) Will use it as reference at the next PR with batch polling updates.