gitkraken
Add fallback/cutoff to our backend calls similar to how we handle GitHub queries (GLVSC-625)
Description
handleException and requestExceptionCount etc. We should stop calling our APIs in a session if there’s a problem and X attempts in a row fail.
Checklist
- [x] I have followed the guidelines in the Contributing document
- [x] My changes follow the coding style of this project
- [x] My changes build without any errors or warnings
- [x] My changes have been formatted and linted
- [x] My changes include any required corresponding changes to the documentation (including CHANGELOG.md and README.md)
- [x] My changes have been rebased and squashed to the minimal number (typically 1) of relevant commits
- [x] My changes have a descriptive commit message with a short title, including a
Fixes $XXX -orCloses #XXX -prefix to auto-close the issue that your PR addresses
@sergeibbb At the high-level I'm confused by the need to have both ServerConnection (existing) and FetchingService (new), why did we need to introduce FetchingService? Why couldn't ServerConnection (or a holistic replacement of it) serve our needs here?
For example, the DraftsService now has a reference to both, and uses them for different usages that feels like it make things more complex.
@eamodio
At the high-level I'm confused by the need to have both ServerConnection (existing) and FetchingService (new), why did we need to introduce FetchingService? Why couldn't ServerConnection (or a holistic replacement of it) serve our needs here?
For example, the DraftsService now has a reference to both, and uses them for different usages that feels like it make things more complex.
Because DraftsService sends requests both to GK API and to S3. It seemed strange to block calls to S3 when GKDev is out.
So, Fetching service is responsible for making any types of requests to different services and the ServerConnection is responsible for making connection to the GK Dev server.
It was discussed here: https://gitkraken.atlassian.net/browse/GLVSC-625?focusedCommentId=123664
But those were already handled differently in ServerConnection, the "raw" fetch method allowed for more general fetching, and the other fetch versions covered more specific cases for the GK API.
Sure, having a "generic" fetch on ServerConnection was always a bit odd, since that class' intent was connecting to our backend, but today that was conceptually still true because the S3 calls are all in service of taking to our backend (just an implementation detail). At the same time, we could move that more generic fetch into a pure utility function.
I'm also unsure of why we wouldn't encapsulate the connection/exception tracking within ServerConnection rather than have it in SubscriptionService. It feels like implementation details are leaking there. And if we change that, do we still need the Retriever?
@eamodio
that more generic fetch into a pure utility function.
Could be. But it needs a link to the container to build the userAgent value.
Anyway, I can join those two classes back and keep that little oddness.
why we wouldn't encapsulate the connection/exception tracking within ServerConnection rather than have it in SubscriptionService.
because it's going to be reset with a new session, but the session is controlled by SubscriptionService. Anyway, it can be moved back to the ServerConnection.
@eamodio
And if we change that, do we still need the Retriever?
We only don't need the retriever, if we stop handling the request error inside of the generic fetch and move the handling to the fetchApi, that is GK specific. That is OK while we don't expect outage or rate-limit problems from S3.
A release has happened. Can I merge?
@sergeibbb Thanks for the reminder! Merged it.
