Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jobs RequestType discriminator seems to not work every #1686

Closed
einari opened this issue Jan 31, 2025 · 6 comments
Closed

Jobs RequestType discriminator seems to not work every #1686

einari opened this issue Jan 31, 2025 · 6 comments
Assignees
Labels
bug Something isn't working jobs Issues related to the job system

Comments

@einari
Copy link
Contributor

einari commented Jan 31, 2025

We're getting exceptions like the following:

Image

@einari einari added bug Something isn't working jobs Issues related to the job system labels Jan 31, 2025
@einari
Copy link
Contributor Author

einari commented Jan 31, 2025

Suggestion to fix:

  • Introduce a IJobRequest interface that all Job request types implements
  • Change the Request on JobState to be of type IJobRequest
  • Custom MongoDB Serializer for IJobRequest interface
  • Remove JobName - as it just reflects the JobType (Type.Name)
  • Change JobType to use Type.Name only
  • Use the JobType (Concept) that we store on the JobState
  • Serializer could have a Map (Dictionary) of JobType enum to actual JobType

@einari einari moved this to Todo in Current Work Jan 31, 2025
@woksin
Copy link
Contributor

woksin commented Jan 31, 2025

JobType now is the assembly fully qualified name and it is actually used to get the correct Grain for an IJob based on the JobState because JobType has an implicit converter to Type. If we change it to Type.Name, the same as JobName, then we cannot do that anymore.

How will the MongoDB Serializer for IJobRequest work? Doesn't it need to know about all the different implementations of IJobRequest? How will it know which CLR Type to convert the BSON document to?

@einari
Copy link
Contributor Author

einari commented Jan 31, 2025

The lock in to the CLR type is actually the biggest problem with this. It would then potentially break a system from one version to another with Jobs being hydrated while upgrading if we change the namespace or name of the job.

We would need to have something that holds a map of JobType string to the CLR Type.

public interface IJobTypeManager
{
     JobType GetJobTypeFor(Type type);
     Type GetClrTypeFor(JobType type);
     Type GetRequestClrTypeFor(JobType type);
}

This could discover anything that implements IJob and also the IJob<TRequest> interface.
By default it would take the CLR Type.Name as the JobType while discovering.

But we could easily support overriding this, which would prove handy when renaming and wanting
to have backwards compatibility.

By introducing a JobTypeAttribute:

[....]
public class JobTypeAttribute(string jobType);

On a Job we would typically then add:

[JobType]
public class CatchUpObserver
{
}

@woksin
Copy link
Contributor

woksin commented Jan 31, 2025

Good idea. That makes sense

@einari
Copy link
Contributor Author

einari commented Jan 31, 2025

And probably even better name for it would be IJobTypes so we don't add those pesky postfixes we don't like :)

@woksin woksin self-assigned this Feb 3, 2025
@woksin woksin moved this from Todo to In Progress in Current Work Feb 3, 2025
@woksin
Copy link
Contributor

woksin commented Feb 4, 2025

Pr her #1693 however I'm having issues understanding how to implement the bson serializer

@woksin woksin closed this as completed Feb 11, 2025
@github-project-automation github-project-automation bot moved this from In Progress to Done in Current Work Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working jobs Issues related to the job system
Projects
Status: Done
Development

No branches or pull requests

2 participants