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

implement Factory and Template patterns #2607

Merged
merged 2 commits into from
Feb 19, 2024

Conversation

ironoa
Copy link
Contributor

@ironoa ironoa commented Feb 17, 2024

https://github.com/w3f/1k-validators-be-PRIVATE/issues/30
#2586

Hey @wpank, let me know what you think of it

Implemented Factory and Template patterns:

  • scorekeeper just startJobs(). It does not decide whether it has to deal with micro or monolith stuff, it's not its duty
  • JobsFactory has the duty to take that decision. The logic must be in there: if(!metadata.config?.redis?.host && metadata.config?.redis?.port)
  • Jobs is an abstract class (it cannot be initialised). Think of it as an interface on steroids. It can implement a common ground skeleton for all the implementations, with specific parts that have to be specified elsewhere (Template)
  • JobsMicroservice and JobsMonolith are extending Jobs indeed, specifying ONLY the missing parts. Checking these 2 files you can clearly see the difference between the Monolith approach and the Micro one.

Open Points:

  • why do we have a jobs folder, and outside of it there is a jobs.ts file ? That doesn't sound right
  • jobs.ts deals with otvWorker. Is otvWorker related to Microservice only stuff or also for Monolith ?
  • better shape/name the relationship between jobs(folder) and cron.ts: jobs -> cron.ts -> (Cron)Jobs .... ?????
    • maybe jobs is not the right name ?
    • in cron.ts sometimes you startXJob and sometimes you startXCron

@ironoa ironoa mentioned this pull request Feb 17, 2024
@wpank
Copy link
Contributor

wpank commented Feb 19, 2024

Thanks, looks good 👍

why do we have a jobs folder, and outside of it there is a jobs.ts file ? That doesn't sound right

This can probably be moved in there - I just didn't touch the jobs.ts file yet, but it makes sense to group them together

jobs.ts deals with otvWorker. Is otvWorker related to Microservice only stuff or also for Monolith ?

This is just the naming convention of the imported functions and whatnot from the Worker package - it's like a namespace for the things exported from it. So it relates to both the monolith and microservice things. The exported things from that pacakage might be named something like Worker.jobs or something along those lines.

@ironoa ironoa merged commit b85501b into will-v3.0.14 Feb 19, 2024
10 checks passed
@ironoa ironoa deleted the scorekeeper-quick-refactor branch February 19, 2024 11:42
@wpank
Copy link
Contributor

wpank commented Feb 19, 2024

@ironoa just to follow up on some of your comments:

Included in #2586 addressed some things you mentioned:

  • that previous jobs.ts file that was outside the folder has been renamed and moved to jobs/cron/WorkerJobs.ts to be more clear that those are wrappers around the jobs imported from the Worker package
  • cron.ts has been renamed and moved to jobs/cron/StartCronJobs.ts
  • in that file all the things that might have been startCron have been renamed to startJob for consistency.

There can probably also been some follow up refactoring and organizing in future PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants