ravendb / quartznet-RavenDB

RavenDB JobStore support for Quartz.NET scheduler.
Apache License 2.0
27 stars 19 forks source link

Can't run jobs when they are defined in program.cs #20

Closed cadilhac closed 2 years ago

cadilhac commented 2 years ago
builder.Services.AddQuartz(q =>
{
    q.UsePersistentStore(storeOptions =>
    {
        storeOptions.UseRavenDb(options =>
        {
            var configuration = new ConfigurationBuilder()
                                    .AddJsonFile("appsettings.json")
                                    .Build();
            var section = configuration.GetSection("QuartzRavenSettings");

            options.Urls = section.GetSection("Urls").Get<string[]>();
            options.Database = section.GetValue<string>("DatabaseName"); ;
            options.CertBytes = section.GetValue<string>("CertBytes");
        });

        storeOptions.UseJsonSerializer();
    });

    // Define a job to grab the exchange rates from an external site
    var jobKey = new JobKey("ExchangeRatesJob", "GlobalJobs");
    q.AddJob<ExchangeRatesJob>(jobKey);
    q.AddTrigger(t => t
        .WithIdentity("ExchangeRatesTrigger")
        .ForJob(jobKey)
        .StartNow()
    );
});

I can't create jobs in asp.net core 6. I get an exception in app.Run():

Quartz.JobPersistenceException: 'The job (DEFAULT.ExchangeRatesJob) referenced by the trigger does not exist.' Why DEFAULT? I used a named group. Using ScheduleJob does the same or variations of identities with groups or not.

Debugging a bit in your code, I saw that RavenJobStore.StoreTrigger is called twice. Once from StoreJobAndTrigger with the correct names and groups, and then another time from ReplaceTrigger. In that last case the jobKey is DEFAULT.ExchangeRatesJob.

Note that when I want to run manually start jobs from a controller action for instance, I have no issue.

Thanks

cadilhac commented 2 years ago

This may be related: In a controller action, I said it was working, but it does not always work. I realized that in controllers, I didn't use a custom job group. In the next example:

IJobDetail job = JobBuilder.Create<ExchangeRatesJob>()
    .WithIdentity("ExchangeRatesJob", "test")   // <--- See "test" here
    .Build();

ITrigger trigger = TriggerBuilder.Create()
    .WithIdentity(Guid.NewGuid().ToString())
    .StartAt(DateBuilder.FutureDate(2, IntervalUnit.Second))
    .ForJob(job.Key)
    .Build();

IScheduler scheduler = await _schedulerFactory.GetScheduler();
await scheduler.ScheduleJob(job, trigger);

I get an exception in RavenJobStore.TriggersFired on this line:

var dbJob = (await session.LoadAsync<Job>(trig.JobKey.GetDatabaseId(), cancellationToken))
                        .Deserialize();

LoadAsync returns null and I can see that trig.JobKey is DEFAULT.ExchangeRatesJob, even if in the database it is set as "JobKey": "ExchangeRatesJob/test". Just before LoadAsync, there is this line:

var trig = trigger.Deserialize();

Strangely, trigger.JobKey is correct but trig.JobKey is wrong with a DEFAULT group.

If you replace .WithIdentity("ExchangeRatesJob", "test") with .WithIdentity("ExchangeRatesJob") then it works.

@ayende @iftahbe Any idea on all this?

Thanks

cadilhac commented 2 years ago

Ah! I think I got it. In this code:

public IOperableTrigger Deserialize()
{
    var triggerBuilder = TriggerBuilder.Create()
        .WithIdentity(Name, Group)
        .WithDescription(Description)
        .ModifiedByCalendar(CalendarName)
        .WithPriority(Priority)
        .StartAt(StartTimeUtc)
        .EndAt(EndTimeUtc)
        .ForJob(new JobKey(JobName, Group))
        .UsingJobData(new JobDataMap(JobDataMap));

you use the group of the trigger when calling ForJob. If both job and trigger share the same group, then it works. The workaround is to modify the code with:

 .ForJob(new JobKey(JobName, JobKey.Split('/')[1]))

This fixes using Quartz in a controller with some custom job group. But it doesn't fix the issue when setting up jobs in program.cs.

ayende commented 2 years ago

We haven't touched this in a while and there isn't enough context here to run things. I assume that this is working for you now?

cadilhac commented 2 years ago

@ayende No. As I said in my last post, it's still impossible to schedule jobs from the asp.net core configuration code in program.cs. The fix for the trigger's group does not help in this case.

My only workaround so far is to configure jobs in the ApplicationStarted event handler.

app.Lifetime.ApplicationStarted.Register(OnStarted);

void OnStarted()
{
    var jobKey = new JobKey("ExchangeRatesJob", "GlobalJobs");

        ISchedulerFactory service = app.Services.GetRequiredService<ISchedulerFactory>();
        IJobDetail job = JobBuilder.Create<ExchangeRatesJob>()
            .WithIdentity("ExchangeRatesJob")
            .Build();

        ITrigger trigger = TriggerBuilder.Create()
            .WithIdentity("ExchangeRatesTriggerNow")
            .ForJob("ExchangeRatesJob")
            .Build();

    Task.Run(async() => {
        IScheduler scheduler = await service.GetScheduler();
        await scheduler.ScheduleJob(job, trigger);
    }).Wait();
}

app.Run();
ayende commented 2 years ago

Can you send a sample project with the failure?

cadilhac commented 2 years ago

As soon as time allows, I will.

cadilhac commented 2 years ago

@ayende Is it me or the version 1.0.6 on nuget is only for .net framework?

cadilhac commented 2 years ago

@ayende Test app is here. It showcases 3 issues.

nefarius commented 2 years ago

@cadilhac your WithIdentity call is missing the custom group name, therefore falling back to DEFAULT.

cadilhac commented 2 years ago

Yes of course. You are not obliged to specify a group. In this case, it's DEFAULT. And when you specify one, it crashes. But please, read my complete description on my repro which explains several issues.

nefarius commented 2 years ago

Yes of course. You are not obliged to specify a group. In this case, it's DEFAULT. And when you specify one, it crashes. But please, read my complete description on my repro which explains several issues.

Yeah, my bad. I cloned your repo and it appeared to work but must've been a glitch, now it crashes reliably 😅

cadilhac commented 2 years ago

Sorry, to be more exact, the bug with the job group has been fixed in my repro. So using one or not won't affect the test. Is the fix enough or did I fix it well, it's another story. The repro is more about seeing the crash when jobs are configured in the asp.net core setup and seeing another issue when using DisallowConcurrentExecutionAttribute.

ayende commented 2 years ago

I reproduced this issue, it looks like there is a change between how the RAM job store vs. the ADO job store is handled. See here:

https://github.com/quartznet/quartznet/blob/90a6bd13b1179277299b032644d4759cbed3e4d9/src/Quartz/Impl/AdoJobStore/JobStoreSupport.cs#L983

Versus:

https://github.com/quartznet/quartznet/blob/90a6bd13b1179277299b032644d4759cbed3e4d9/src/Quartz/Simpl/RAMJobStore.cs#L447

It looks like it is updating the trigger, and then the sequence of events (of RescheduleJob) hits this issue.

Given your workaround, I don't think this matters too much. It looks like primarily an issue with Quartz extensions. We'll be happy to accept a PR, but I don't think we're going to focus on that in the near future.

cadilhac commented 2 years ago

In that case, if you don't want to fix it, please modify your README. With this line, where it's set:

// Add jobs and triggers as you wish

it implies that one can configure the jobs in the setup part of asp.net core. And it's not obvious when you transition from RAM store to RavenDb store that it will fail.

cadilhac commented 2 years ago

Also why don't you integrate the single currently active PR that fixes some serious bugs. I also showed you a fix that allows using custom job groups instead of DEFAULT. It would be great in a version 2.0.x under NuGet for .Net core (currently only 1.0.6 for .Net Framework).

ayende commented 2 years ago

You are correct, this is fixed now.

cadilhac commented 2 years ago

@ayende This is great. Thank you. However:

  1. Several issues were mentioned in this thread but the original one is not fixed. If you specify a job goup, it will crash. Check this.
  2. Do you plan a nuget release so that there is a lib for .net core?

Thanks.

ayende commented 2 years ago

Sorry, I'm trying this again, and I can't recall what the current status is. I run your repro above with the latest code and it doesn't reproduce the exception.

cadilhac commented 2 years ago

Well, if I download the latest code and if I use custom group names for jobs and/or triggers, then an exception is still thrown.

The probleme is still at this line where you create a job key with the job name and the trigger's group, instead of the job name and the job's group.

Btw, in Trigger.cs, I see this:

        public string Name { get; set; }
        public string Group { get; set; }
        public string Key => $"{Name}/{Group}";

        public string JobName { get; set; }
        public string JobKey { get; set; }

Why not this:

        public string Name { get; set; }
        public string Group { get; set; }
        public string Key => $"{Name}/{Group}";

        public string JobName { get; set; }
        public string JobGroup { get; set; }
        public string JobKey => $"{JobName}/{JobGroup}";
ayende commented 2 years ago

Those are design decisions from many years ago.

Your repro code isn't showing the problem, however?

What would be your suggested fix?

cadilhac commented 2 years ago

Those are design decisions from many years ago.

I understand but this seems odd. Why store JobName in that case? It's also stored in the JobKey property... The best would be to have a Name and a Group properties.

Your repro code isn't showing the problem, however?

Yes it is. I'm using a "GlobalJobs" job group and this is what makes it fail:

var jobKey = new JobKey("ExchangeRatesJob", "GlobalJobs");

What would be your suggested fix?

As I said previously, I did a workaround by replacing this line with:

.ForJob(new JobKey(JobName, JobKey.Split('/')[1]))

This does not look very nice but since it seems there is no other way to access the job's group which is not directly stored in a JobGroup property...