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

Betterment/update job #134

Merged
merged 22 commits into from
Oct 9, 2024
Merged

Betterment/update job #134

merged 22 commits into from
Oct 9, 2024

Conversation

heemankv
Copy link
Contributor

@heemankv heemankv commented Sep 30, 2024

This PR solves #112.

  • Remove update_job_status, update_metadata, update_job_optimistically, post_job_update functions from Database implementation.
  • Reimplemented update_job to use JobItemUpdates.
  • Provide JobItemUpdates object with all the needed changes only, which get's reflected on the database.

WHY this approach?

  • Code had three functions update_job, update_job_status & update_metadata to update job values in database.
  • update_job was being passed with the whole jobItem to update, in it's usage it was only changing 2-3 values in the job and not the whole job object, resulting in other values unnecessarily being rewritten. (for eg, same version value is being sent within jobItem, resulting in a pointless rewrite 1 -> 1).
  • The above is also why the code had to update version and updated_at fields post these updates, since first the version get's rewritten (1->1) and then it's overwritten to an increased value in post_job_update (1->2).
  • update_job_status and update_metadata followed the same trend but only updated individual values.

WHAT now ?

  • Trimming the above problem, reinventing update_job seemed the best approach to move forward with.
  • Provide a JobItemUpdates object that consist of all the updates that needs to be done to the job in a single query.
  • Code doesn't allow updating id and created_at.
  • Code implemented only_status and only_metadata to allow for cleaner code for these updates.
Screenshot 2024-09-30 at 4 49 48 PM Screenshot 2024-09-30 at 4 51 09 PM

@heemankv heemankv self-assigned this Sep 30, 2024
@heemankv heemankv linked an issue Sep 30, 2024 that may be closed by this pull request
@heemankv heemankv added the enhancement New feature or request label Sep 30, 2024
@heemankv
Copy link
Contributor Author

Done with Self Review,
I think we are missing a test case for the new update_job.
WDYT ? @apoorvsadana

@coveralls
Copy link

coveralls commented Sep 30, 2024

Coverage Status

coverage: 67.817% (-0.03%) from 67.848%
when pulling b4e4a4b on betterment/update_job
into ba2aeb8 on main.

Copy link
Contributor

@apoorvsadana apoorvsadana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks cleaner but I have two issues before the reivew

  1. The version shouldn't come from the user. It's a invisible implementation that you don't need to worry about when writing the code. All you should know is that if your job is stale the update won't work.
  2. If tomorrow I add a new field to Job and call update_job, it might feel that it would just work but actually I would need to update the logic and handle the if else. Is there a way to catch this silent failure later?

@heemankv
Copy link
Contributor Author

heemankv commented Oct 1, 2024

  1. The version shouldn't come from the user. It's a invisible implementation that you don't need to worry about when writing the code. All you should know is that if your job is stale the update won't work.

Updated the logic to don't let parent fn to decide whether to update version or not, it will now always be updated.
Additionally, the same logic is now followed for updated_at.

crates/orchestrator/src/jobs/types.rs Outdated Show resolved Hide resolved
crates/orchestrator/src/jobs/types.rs Outdated Show resolved Hide resolved
crates/orchestrator/src/jobs/types.rs Outdated Show resolved Hide resolved
crates/orchestrator/src/jobs/mod.rs Outdated Show resolved Hide resolved
crates/orchestrator/src/tests/database/mod.rs Show resolved Hide resolved
crates/orchestrator/src/tests/database/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@ocdbytes ocdbytes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@heemankv heemankv enabled auto-merge (squash) October 9, 2024 16:58
@heemankv heemankv merged commit 63e4fa1 into main Oct 9, 2024
9 checks passed
@heemankv heemankv deleted the betterment/update_job branch October 9, 2024 17:48
ocdbytes pushed a commit that referenced this pull request Oct 16, 2024
* update: JobItemUpdates

* update: changelog

* update: e2e fixed

* update: tests comments removed

* update: cleaning print statement

* update: version, updated_at will always automatically be updated

* update: version based job fetch test case

* fix: build

* update: simplified impl for update_job

* fix: lint

* update: false call check handled

* test update: panic if job is not found

* update: cleaned test case for db update_job

* update: shifted to_document to mongodb

* update: builder pattern for jobitemupdates

* update: default->new

* update: lint fix

* update: fixed fatal error

* update: match statement corrected
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Database update logic fix
4 participants