HangfireIO / Hangfire

An easy way to perform background job processing in .NET and .NET Core applications. No Windows Service or separate process required
https://www.hangfire.io
Other
9.4k stars 1.7k forks source link

[Hangfire.DynamicJobs] [Question] What is best-practice when assigning a job to a queue? #2259

Open Malgefor opened 1 year ago

Malgefor commented 1 year ago

Hi! I noticed that you can both use the overload of AddOrUpdateDynamic-method or the DynamicRecurringJobOptions to set a specific queue for a job. What is best-practice here?

jobManager.AddOrUpdateDynamic(
    "MyJobTitle",
    "myqueue",
    () => DoStuff(),
    "* * * * *",
    new DynamicRecurringJobOptions
    {
        Filters = new List<JobFilterAttribute>
        {
            new QueueAttribute("myqueue"),
            new DisableConcurrentExecutionAttribute(int.MaxValue)
        }
    });

Semi-related question/observation: I see in the HangFire.Hash SQL database table that the Field-column with value Queue is always set to default, even though my code sets it to myqueue. However, the Field-column with value Job does contain the correct queue configuration.

Field Value
Job {"t":"Hangfire.DynamicJob, Hangfire.DynamicJobs","m":"Execute","p":["Hangfire.DynamicJob, Hangfire.DynamicJobs","Hangfire.Server.PerformContext, Hangfire.Core"],"a":["{\"t\":\"MyProject.MyJob, MyProject\",\"m\":\"ExecuteJob\",\"p\":\"[]\",\"a\":\"[]\",\"f\":[{\"$type\":\"Hangfire.QueueAttribute, Hangfire.Core\",\"Queue\":\"myqueue\",\"Order\":2147483647},{\"$type\":\"Hangfire.DisableConcurrentExecutionAttribute, Hangfire.Core\",\"TimeoutSec\":2147483647}]}",null]}
Queue default
timpikelmg commented 7 months ago

To add a note to this since I have ran into a similar issue recently, I believe the best practice is to use the queue parameter in the AddOrUpdateDynamic call but it looks like there is a bug in the DynamicJobs where it is not using the queue you provide in the parameter and you have to set the one in the options otherwise it does not work.

In this code, I believe it should be include the queue in the Job.FromExpression since while it currently adds the queue to the underlying job, by not setting in the DynamicJob here, everything runs under the default queue (the options appears to set this properly somewhere else which is why it works with that).

    private static Job ToDynamicJob([NotNull] Job job, [CanBeNull] IEnumerable<JobFilterAttribute> filters, [CanBeNull] string queue = null)
    {
        ArgumentNullException.ThrowIfNull(job);

        var invocationData = InvocationData.SerializeJob(job);
        var dynamicJob = new DynamicJob(invocationData.Type,
            invocationData.Method,
            !string.IsNullOrEmpty(invocationData.ParameterTypes) ? invocationData.ParameterTypes : null,
            invocationData.Arguments,
            filters?.ToArray());

        return Job.FromExpression(() => DynamicJob.Execute(dynamicJob, default), queue);
    }
timpikelmg commented 7 months ago

Created PR in Hangfire.DynamicJobs repository to try to resolve the issue.

For anyone running into the issue, the best workaround I have found is either setting the QueueName in DynamicRecurringJobOptions (note this does not appear to use the queue name in retries when used like this so retries go back to default queue) or better to set the Queue attribute on the job class/method since this does get used in retries.

iamSathishMohan commented 1 month ago

@timpikelmg Setting the QueueName in DynamicRecurringJobOptions doesn't work in my case. When can i expect the fix to be released ?

timpikelmg commented 1 month ago

@timpikelmg Setting the QueueName in DynamicRecurringJobOptions doesn't work in my case. When can i expect the fix to be released ?

In the end I had to use the [Queue("queuename")] attribute to make it work for the system I have built. There is a PR to with a proposed fix for the issue in the DynamicJobs repository but I don't believe @odinserj has had a chance to look at it.

fr4gles commented 2 weeks ago

It would be nice if hangfire adds a first class solution for MAMQ / dynamic jobs. Currently it holds us from migrating to hangfire 1.8.x