Conversation
by setting up a fetching layer that is used by both DraftsService (code suggestions via S3) and ServerConnection (GK API) GLVSC-625 #3494
9ab6c88 to
c873f7c
Compare
just track the count and stop when there are too many errors GLVSC-625 #3494
| options?: { token?: string; organizationId?: string }, | ||
| ): Promise<Response> { | ||
| if (this.requestsAreBlocked) { | ||
| throw new Error('Requests are blocked'); |
There was a problem hiding this comment.
We're throwing an error now in places where we weren't before. Should we be catching/handling this from the callers? And if not, have we verified that it doesn't break anything?
There was a problem hiding this comment.
fetchApi is private inside of SubscriptionService and in all 3 places where it's called all exceptions are caught now.
But, we probably can handle it differently from other types of exceptions. For example, when checkInAndValidateCore catches and exception it changes subscription status. Maybe we shouldn't do it for this type of error?
There was a problem hiding this comment.
@axosoft-ramint
Well, what I see is that it has started hitting debugger expressions that haven't been hit before. Here is an example on the picture, but probably, there are more such places. Should we do anything about it now or it can be postponed?
There was a problem hiding this comment.
That's probably fine? Before these changes, it would hit this anyway if the fetch threw an error, so really the only new error we're introducing that hits this debugger is from too many failed requests, in which case, we would probably want to hit this line and know about it anyway.
|
@sergeibbb At the high-level I'm confused by the need to have both For example, the |
Because 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? |
Could be. But it needs a link to the container to build the Anyway, I can join those two classes back and keep that little oddness.
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. |
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 |
just track the count and stop when there are too many errors GLVSC-625 #3494
6661f2e to
fdb923e
Compare
827e97a to
daf9e3b
Compare
by setting up a fetching layer that is used by both DraftsService (code suggestions via S3) and ServerConnection (GK API) GLVSC-625 #3494
85bf10c to
6487b77
Compare
6487b77 to
81e4a4b
Compare
just track the count and stop when there are too many errors GLVSC-625 #3494
81e4a4b to
7a693a7
Compare
just track the count and stop when there are too many errors GLVSC-625 #3494
ramin-t
left a comment
There was a problem hiding this comment.
Latest version looks good. I didn't really have time to exhaust all the possibilities but what I did simulate, it handles well. Thanks for working on it!
Let's wait to merge this until after our patch release, because it's targeting main and I would like this to sit in the prerelease for longer before we release it.
|
Hi @axosoft-ramint |
@sergeibbb Thanks for the reminder! Merged it. |
Description
handleExceptionandrequestExceptionCountetc. We should stop calling our APIs in a session if there’s a problem and X attempts in a row fail.Checklist
Fixes $XXX -orCloses #XXX -prefix to auto-close the issue that your PR addresses