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

refactor: improve branch cache logic #17538

Closed

Conversation

RahulGautamSingh
Copy link
Collaborator

@RahulGautamSingh RahulGautamSingh commented Aug 31, 2022

Context

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please tick one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

- shift git-conflict to branch cache
- invalidate cached values when SHAs change (not central)
@RahulGautamSingh RahulGautamSingh marked this pull request as draft August 31, 2022 11:54
@RahulGautamSingh RahulGautamSingh changed the title fix: update branch cache update logic fix: branch cache update logic Aug 31, 2022
@RahulGautamSingh RahulGautamSingh requested review from zharinov, rarkins and viceice and removed request for zharinov and rarkins September 1, 2022 08:23
@RahulGautamSingh RahulGautamSingh marked this pull request as ready for review September 1, 2022 11:18
@RahulGautamSingh RahulGautamSingh changed the title fix: branch cache update logic fix: update branch caching logic Sep 6, 2022
@RahulGautamSingh RahulGautamSingh changed the title fix: update branch caching logic fix: improve branch cache logic Sep 7, 2022
@RahulGautamSingh RahulGautamSingh marked this pull request as draft September 7, 2022 06:53
@RahulGautamSingh RahulGautamSingh marked this pull request as ready for review September 8, 2022 06:30
@RahulGautamSingh RahulGautamSingh changed the title fix: improve branch cache logic refactor: improve branch cache logic Sep 9, 2022
@RahulGautamSingh
Copy link
Collaborator Author

Current Flow:

  1. Initialize branchCache = {}
  2. if(repositoryCache === enabled && branchExisted)
  • branchCache = findBranchCache(branchName)

if cache found assign it to branchCache
else create new branchCache with the fields branchName+sha, baseBranchName+sha

  • syncBranchCache()
    -- processBranch() --
  1. if new commit is made -> setBranchCommit()

Note:
setX fns logic flow:

  1. try to find branchCache

if found update isX value
else return

not creating new branchCache here because if we wanted cache we would have done so before processing branch

@RahulGautamSingh
Copy link
Collaborator Author

RahulGautamSingh commented Sep 13, 2022

I made a tiny modification:

  • setBranchCommit and syncBranchCache have been converted to pure functions.
    Reason: This is good practise.

Will revert if you also think that it makes the code appear more complicated and less readable.

Aside from the two concerns:

  1. using pure fns and
    2. Not creating branchCache and just returning when branchCache is not found in setX fns

I believe the implementation is in line with what you requested in the (#17663) 🤞.

I tested it on repos, and it works as intended.

@rarkins
Copy link
Collaborator

rarkins commented Sep 13, 2022

I think there's no harm in "writing" to the branch cache during a run even when cache is disabled as long as the cache file itself isn't written out at the end. We can simply think of it as branch state, which either gets written to cache if enabled or discarded.

@RahulGautamSingh
Copy link
Collaborator Author

RahulGautamSingh commented Sep 14, 2022

New updates:
To get in line with the branch state terminology, I made some changes:

  1. assign cached value to branch if it exists, (same as old)
  2. if cache not found then create a minimal branchCache in the beginning only ie. before syncing cache

Minimal BranchCache:

{
branchName,
sha, 
baseBranchName, 
baseBranchSha
 }

After that we continue with : syncBranchCache -> processBranch -> if new commit --> setBranchCommit()

Note:
Now we have a minimal branchCache before processing branch in all these cases too:

  1. branch cache not found
  2. on first run ie. branchExisted=false
  3. repositoryCache !== enabled

@RahulGautamSingh
Copy link
Collaborator Author

Dividing this PR into smaller PRs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve branch cache logic
3 participants