feat(GGs): introduce GitFileSystemService as middleman interface #867
Conversation
dcshzj
changed the title
There was a problem hiding this comment.
Clarifications:
is this already funcitonal in local dev? is there a quick sanity check that I can do here to assert this?
[Style, Error Handling]: Lets not return an unknown, it would resolve to the top level, and it might not yield the usefulness of the types!
| } | ||
|
|
||
| // Pull the latest changes from upstream Git hosting provider | ||
| // TODO: Pulling is a very expensive operation, should find a way to optimise |
There was a problem hiding this comment.
This is an interesting note, out of curiosity why is this the case ah? I thought after the first git clone, the git pull should be ok no?
How expensive is this?
There was a problem hiding this comment.
Expensive in terms of latency? Latency of network call or disk write? Curious - did you manage to benchmark this on ur local?
There was a problem hiding this comment.
I didn't manage to dive into the exact cause of the latency issues, but just a quick benchmarking on my local machine, the Git pull operation adds almost 2 seconds in latency for each request (281 ms without, 2.91 seconds with pulling)
There was a problem hiding this comment.
As discussed, let's focus on latency once MVP is out as our baseline metric is the reduction of API calls
There was a problem hiding this comment.
I thought git checkout might help and I ran a simple benchmark, weirdly checkout is slower than pull?
❯ node benchmark.js
Time taken for git checkout: 3386.3763749599457 ms
Time taken for git pull: 3241.95516705513 ms
❯ node benchmark.js
Time taken for git checkout: 3356.351875066757 ms
Time taken for git pull: 3228.0880839824677 ms
❯ node benchmark.js
Time taken for git checkout: 3306.690124988556 ms
Time taken for git pull: 3079.9089580774307 ms
❯ node benchmark.js
Time taken for git pull: 2990.787334084511 ms
Time taken for git checkout: 3404.82679104805 ms
| ) | ||
| } | ||
|
|
||
| return okAsync(null) |
There was a problem hiding this comment.
why return okAsync(null) over the path to the repo? (okAsync(path_to_repo))
| ) | ||
| } | ||
|
|
||
| return okAsync(null) |
There was a problem hiding this comment.
also, why not do a git fetch here? the repo is correct already but not guaranteed to be updated
| ) | ||
| } | ||
|
|
||
| return okAsync(null) |
There was a problem hiding this comment.
Does the git fetch help? In any read operation we are already performing a git pull first
| } | ||
|
|
||
| // Pull the latest changes from upstream Git hosting provider | ||
| // TODO: Pulling is a very expensive operation, should find a way to optimise |
There was a problem hiding this comment.
Expensive in terms of latency? Latency of network call or disk write? Curious - did you manage to benchmark this on ur local?
mergify
bot
deleted the
IS-320-create-new-interface-for-local-git-service-3
branch
Problem
Closes IS-320.
Solution
Breaking Changes
Features:
This PR is the first of a 2-part PR. The eventual architecture should look something like this:
Tests
Deploy Notes
New environment variables:
EFS_VOL_PATH: The path to the EFS volume used for storing the Git repositoriesNew dependencies:
simple-git: Wrapper for running Git-related operations on the underlying file systemNew dev dependencies:
mock-fs: Provides a mock filesystem when performing unit tests