icsharp / Hangfire.RecurringJobExtensions

Extensions for Hangfire to build RecurringJob automatically
MIT License
181 stars 36 forks source link

Same job type with different job-data skipped #2

Closed ludzzz closed 7 years ago

ludzzz commented 7 years ago

Hi,

I got an issue when I setup several Recurring Job from the same type but with different parameters.

The way the JobFilter is storing the Recurring Job Info in ExtendedDataJobFilter.cs L.29 avoid us to use the same type of Job more than once. We are using a Dictionnary with the Key as a string generated from GetRecurringJobId which is using the the Type and the method name (same as the implementation of the toString in Job class in Hangfire which make the link).

In case of several job from the same type with different job-data, we only have the first one added the other one are skipped.

I tried to figured out how to solve that, but i don't have yet a good solution to offer. Might be use the argument property of the Job, But its not very clean I think.

I will check if we can do something with the RecurringIdJob but sounds not good and huge breaking change, because Its the Id used in Hangfire itself.

Did i do something wrong? Any input to help to solve this issue?

Edit: we can't use as well the RecurringJobId straight from the json config because we are losing it when we got the job context back in the OnPerrforming , its why I thought to use the job argument when we are adding/updating it .

icsharp commented 7 years ago

@gyzmau Thanks for your feedback. When we use json config ,the json settings token job-name is equals to RecurringJobId. As your said to store recuringjob info with the key JobId by extension method GetRecurringJobId cannot distinguish same job-type, because same job type has the same JobId with GetRecurringJobId. I think using the specified job-name as the key is proper, it will do this later.

ludzzz commented 7 years ago

Edit: i did this reply without seeing your previous comment, i will reply about your suggestions, not sure we have the job-name in the OnPerforming .

I think we should have two ways to solve this problem:

The first one is cleaner but ask for a big refactor in the way that RecurringJobExtensions is working.

The second one is less less evasive but more dirty in the concept. And I dunno enough about Hangfire and the Args property to know if we can use it.

Do you have any suggestion @icsharp ?

icsharp commented 7 years ago

@gyzmau Hangfire add/update recurringjob depends on recurringJobId, if you know the the Hangfire class RecurringJob static method:

public static void AddOrUpdate(string recurringJobId, Expression<Action> methodCall, Func<string> cronExpression, TimeZoneInfo timeZone = null, string queue = "default");

The first parameter recurringJobId is the identifier of recurringjob, so the key point is how to distinguish job by recurringJobId.

For your first way, it is noting to do with hangfire storage. RecurringJobExtensions provider the way to define the recurringjob info and job-data info in json file, or you can impl the interface IConfigurationProvider to custom your storage to store you recurringjob info.

The second way as you mentioned , maybe it is proper to use RecurringJobId as job-name in json file to instead Args property.

Maybe your scenario can simply to define two different recurringjob to avoid the same job type.

icsharp commented 7 years ago

@gyzmau Maybe it is also not proper to use job-name as the key, because it cannot find it back in PerformContext. Simply way is to define two different recurringjob to avoid the same job type.

ludzzz commented 7 years ago

@icsharp yes I confirm about the performContext

I agree it will be simpler to do different types of Job. But if i do that I will lost completely the benefit of RecurringJobExtension (In the way i see it).

For example if you have simple task to do on three different countries. It will be very counterproductive to do a RecurringJob class by country for the same task. if I have to do job "strong typed", It will be the same at the end as Hangfire, I like the fact that i just code a simple task that i can apply to different parameters.

I could as well, do one Job who take care of all, but again i like the flexibility that you can launch only one country, especially in case of failure (you can relaunch one).

I need to go a bit further and dig a bit deeper to have better idea. It might be an issue on hangfire design as well (Totally an assumption i dont know at all hangfire codebase).

icsharp commented 7 years ago

@gyzmau I have checked the hangfire source code, all jobs(Fire-and-forget jobs/Delayed jobs/Recurring jobs etc.) are fetched from storage and have the same job payload. Hangfire server executed job depends on job method and its args. In other words, the PerformContext is the common design for all jobs, not only the recurring jobs. For your example, I have not a good idea to solve yet, maybe you can impl your business logic in another way.

ludzzz commented 7 years ago

@icsharp yes i reached the same conclusion, Its more by design on the Hangfire side.

I am trying to figure out if i can use the Args of a job.

Thanks a lot for having a look at it!

icsharp commented 7 years ago

@gyzmau I have fix it , see v1.1.5.

ludzzz commented 7 years ago

Oh awesome! I will check tomorrow. Thanks you a lot!