marcoCasamento / Hangfire.Redis.StackExchange

HangFire Redis storage based on original (and now unsupported) Hangfire.Redis but using lovely StackExchange.Redis client
Other
452 stars 108 forks source link

Wrong queue procesing order? #81

Closed xumix closed 4 months ago

xumix commented 5 years ago

I have my options defined like this:

                {
                    Queues = new[]
                                     {
                                         Enum.GetName(typeof(PriorityEnum), PriorityEnum.Critical).ToLower(),
                                         Enum.GetName(typeof(PriorityEnum), PriorityEnum.Important).ToLower(),
                                         Enum.GetName(typeof(PriorityEnum), PriorityEnum.Default).ToLower(),
                                         Enum.GetName(typeof(PriorityEnum), PriorityEnum.Minor).ToLower()
                                     }
                };

But in the Dashboard they are displayed like this: 2019-01-15 12 48 27 queues - hangfire - mozilla firefox

The order in redis is the same: 2019-01-15 12 48 58 redis commander_ home - mozilla firefox

Looking into FetchedJobsWatcher.cs it seems that the queues are processed in wrong order

marcoCasamento commented 5 years ago

Why do you say that the code in FetchedJobsWatcher.cs is wrong ? it process the queues in the same order as they are returned from the redis set.

To me it seems that the problem relies in the order items appears in the queue. I haven't seen that problem in my installations, is it possible that the queues have been added on subsequent start of the application ? could you try to delete the key hangfire:PpProd:queues and restart the application ?

xumix commented 5 years ago

Why do you say that the code in FetchedJobsWatcher.cs is wrong ? It is not wrong itself, yes, it looks like by some reason the queues are add in wrong order

is it possible that the queues have been added on subsequent start of the application ? We launch many instances at once, so maybe. How can I sort this problem?

Lexy2 commented 4 months ago

The queues are added to a set that does not maintain any specific order. When jobs are processed, the queues are checked one after another,

and no specific order is guaranteed.

EDIT: This turned out to be wrong. The jobs are processed in the order in which queues are declared.

A mitigation in your case could be to dedicate a specific backgroundprocessingserver to a queue with more workers and to less prioritized queues with fewer workers.

Lexy2 commented 4 months ago

Or, actually, implement an IElectState filter that prevents moving them from Enqueued state to Processing state if there are jobs with higher priority.

xumix commented 4 months ago

The queues are added to a set that does not maintain any specific order. When jobs are processed, the queues are checked one after another, and no specific order is guaranteed. A mitigation in your case could be to dedicate a specific backgroundprocessingserver to a queue with more workers and to less prioritized queues with fewer workers.

Hangfire docs say that a queue priority is defined by the order it is added in the code. It is reasonable for me to assume that the storage provider is maintaining the described behavior

marcoCasamento commented 4 months ago

(very) old issue. I actually believe that @Lexy2 is right in saying that the redis set does not maintain any specific order and this is the root cause of the problem. I also agree with @xumix, we should honor hangfire described behavior, so the queue should be processed in the order they appear in configuration.

Hangfire store Queues in a BackgroundJobServerOptions instance. Anyone knows how to access that instance ? we could then sort the result of the set using BackgroundJobServerOptions.Queues order.

Lexy2 commented 4 months ago

I guess I could think of something for it. It could be a weighted set, like for scheduled jobs, or a list. But then this could be a breaking change as the underlying data structures would have to change.

marcoCasamento commented 4 months ago

I agree with you that a ZSET would be the most efficient data structure to use here, but would then need a breaking change.

Giving that the queues that a server can process remain the same until the server restart, what if we we load a ZSET on startup, basing on the value configured in BackgroundJobServerOptions.Queues ? (again, I dont know how to get that list, other than having the user explicitly pass in configuration, but that would be another change)

Lexy2 commented 4 months ago

The servers that process a specific queue would have no say in what job gets assigned to them next. It's the FetchNextJob override of the IStorageConnection that decides that. But again, it accepts an array of queues, which, I would expect, be sorted in the declaring order, regardless of the order those queues are displayed in the monitoring API.

I'll test that and get back to you. It may turn out that there is no issue.

marcoCasamento commented 4 months ago

Thank you for your effort, appreciated. You're then supposing that the order of queue in RedisConnection.FetchNextJob queues argument is already correct ? That would means that there are no issue. I don't use hangfire with multilple queue on a single instance, so I can't tell if it is an issue.

xumix commented 4 months ago

That would means that there are no issue.

There is an issue at least in the displayed order. It is an old issue and I'm unable to check if it persists now, but AFAIR the issue was not only in the display order but also in the processing order (Because it would be really a non-issue for me if it was just a display order problem)

Lexy2 commented 4 months ago

Okie-dokie, so turns out there is nothing wrong with the processing order. I scheduled 400 jobs, 100 jobs for each queue, starting from the minor up to critical, but declared queues the other way around, starting from critical to minor.

The jobs were processed exactly in the order the queues were declared.

When there are fewer jobs than workers, jobs from the lower priority queues get processed next, but only in strict order of queues declared.

My code:

using Hangfire.Redis.StackExchange;
using Hangfire;

var builder = Host.CreateApplicationBuilder(args);
builder.Services.AddHangfire(cfg => {
    // cfg.UseInMemoryStorage();
    cfg.UseRedisStorage("localhost:6379", new RedisStorageOptions {
        Db = 2,
        Prefix = ""
    });
});

string[] queues = ["critical", "important", "default", "minor"];

var host = builder.Build();
await host.StartAsync();

var jobClient = host.Services.GetRequiredService<IBackgroundJobClientV2>();

for (int i = queues.Length-1; i >= 0; i--)
{
    for (int j = 0; j < 100; j++)
        jobClient.Enqueue(queues[i], () => Console.WriteLine($"Hello from {queues[i]}"));
}

var storage = host.Services.GetRequiredService<JobStorage>();

using var server = new BackgroundJobServer(new BackgroundJobServerOptions {
    Queues = queues
}, storage);

Console.ReadKey();

server.SendStop();

await server.WaitForShutdownAsync(default);

await host.StopAsync();

Which server and which worker is going to process the job is not determined by the storage.

Also, jobs from lower priority queues may finish earlier than jobs from higher priority queues, and it may leave an impression that the lower priority queue jobs were enqueued first. Which is not true and can be checked using the monitoring API. You can see that I also tested with memory storage, and the job messages overlapped more there than they did in Redis.

Conclusion: Hangfire.Redis.StackExchange works as expected in this regard.

Issue can be closed.

Lexy2 commented 4 months ago

There is an issue at least in the displayed order.

Indeed there is. It's because when dashboard queries the monitoring API, the storage returns the queue set, where order is not guaranteed.

AFAIR the issue was not only in the display order but also in the processing order (Because it would be really a non-issue for me if it was just a display order problem)

I can't say anything about the versions of Hangfire and Hangfire.Redis.StackExchange libraries active at the time, but today it processes stuff in the correct order, as I tested.

marcoCasamento commented 4 months ago

I guess this should now be closed. @xumix if you happen to make a repro, feel free to reopen.