Revert to long polling in the ServalClientHelper. Fixes #98. #143

Enkidu93 · 2023-09-26T20:24:11Z

I think this is the intended behavior. The only change I'm realizing I should possibly make is to make the Exception more specific (?).

This change is

johnml1135 · 2023-09-27T14:35:30Z

tests/Serval.E2ETests/ServalClientHelper.cs line 132 at r1 (raw file):

                revision++;
            }
            catch

The only thing we should be allowing this to continue on is timeouts - can you check the status code? Will the exception type be different? If it is another error, we should just rethrow.

johnml1135

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ddaspit and @Enkidu93)

Enkidu93 · 2023-09-27T15:07:08Z

tests/Serval.E2ETests/ServalClientHelper.cs line 132 at r1 (raw file):
                revision++;
            }
            catch
The only thing we should be allowing this to continue on is timeouts - can you check the status code? Will the exception type be different? If it is another error, we should just rethrow.

Yep, my apologies. I can do that.

johnml1135 · 2023-09-27T16:09:44Z

tests/Serval.E2ETests/ServalClientHelper.cs line 132 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Yep, my apologies. I can do that.

if (status_ == 408)
{
  string responseText_ = ( response_.Content == null ) ? string.Empty : await response_.Content.ReadAsStringAsync().ConfigureAwait(false);  throw new ServalApiException("The long polling request timed out.  Did you start the build?", status_, responseText_, headers_, null);
}

ddaspit

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Enkidu93)

tests/Serval.E2ETests/ServalClientHelper.cs line 130 at r2 (raw file):

                    break;
                }
                revision++;

We want to use the revision from the returned build model.

johnml1135

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93)

ddaspit

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Enkidu93)

Enkidu93 · 2023-09-28T13:28:16Z

tests/Serval.E2ETests/ServalClientHelper.cs line 132 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

if (status_ == 408)
{
  string responseText_ = ( response_.Content == null ) ? string.Empty : await response_.Content.ReadAsStringAsync().ConfigureAwait(false);  throw new ServalApiException("The long polling request timed out.  Did you start the build?", status_, responseText_, headers_, null);
}

Ultimately, this is what is thrown here. Although it's a bit janky, checking based on the message ought to be reliable. Is this OK with you, John?

johnml1135 · 2023-09-28T14:58:50Z

tests/Serval.E2ETests/ServalClientHelper.cs line 132 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Ultimately, this is what is thrown here. Although it's a bit janky, checking based on the message ought to be reliable. Is this OK with you, John?

The ServalApiException has a StatusCode - can you check that instead?

johnml1135

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @ddaspit)

johnml1135 · 2023-09-28T15:00:05Z

tests/Serval.E2ETests/ServalClientHelper.cs line 129 at r4 (raw file):

                }
                revision = result.Revision;
            }

revision = result.Revision + 1; // look for the next revision, the current one

johnml1135

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93)

ddaspit

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johnml1135)

tests/Serval.E2ETests/ServalClientHelper.cs line 129 at r4 (raw file):

Previously, johnml1135 (John Lambert) wrote…

revision = result.Revision + 1; // look for the next revision, the current one

He is passing revision + 1 to GetBuildAsync.

…dev/serval into revert_to_long_polling_#98

Enkidu93 · 2023-09-28T15:32:33Z

tests/Serval.E2ETests/ServalClientHelper.cs line 132 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

The ServalApiException has a StatusCode - can you check that instead?

I tried that initially, but that's not actually what gets thrown here. There are a few timeouts at play here. As I understand it, this timeout is actually the timeout thrown by the HTTP client itself. I am realizing though that this timeout is configured to only 10 seconds. Should we reconfigure it to be greater than the longpolltimeout option?

Enkidu93

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johnml1135)

tests/Serval.E2ETests/ServalClientHelper.cs line 129 at r4 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

He is passing revision + 1 to GetBuildAsync.

If it would be clearer to do otherwise, that's fine. But, of course, it has the same effect.

ddaspit

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johnml1135)

tests/Serval.E2ETests/ServalClientHelper.cs line 129 at r4 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

If it would be clearer to do otherwise, that's fine. But, of course, it has the same effect.

I don't have an opinion either way.

johnml1135 · 2023-09-29T14:10:51Z

tests/Serval.E2ETests/ServalClientHelper.cs line 129 at r4 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I don't have an opinion either way.

I didn't see it - it is clear enough.

johnml1135 · 2023-09-29T14:12:33Z

tests/Serval.E2ETests/ServalClientHelper.cs line 132 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

I tried that initially, but that's not actually what gets thrown here. There are a few timeouts at play here. As I understand it, this timeout is actually the timeout thrown by the HTTP client itself. I am realizing though that this timeout is configured to only 10 seconds. Should we reconfigure it to be greater than the longpolltimeout option?

Absolutely - we would want it to present itself as a proper message. We could also change it by having the long-poll timeout be under the http timeout, say 9 seconds? @ddaspit what do you think?

johnml1135

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Enkidu93)

ddaspit

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Enkidu93)

tests/Serval.E2ETests/ServalClientHelper.cs line 132 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

Absolutely - we would want it to present itself as a proper message. We could also change it by having the long-poll timeout be under the http timeout, say 9 seconds? @ddaspit what do you think?

Yes, we should set the request timeout for HttpClient to something higher than the long polling timeout. If we want a shorter timeout for other requests, we can control that using the cancellation token.

Enkidu93 · 2023-09-29T20:14:03Z

tests/Serval.E2ETests/ServalClientHelper.cs line 132 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Yes, we should set the request timeout for HttpClient to something higher than the long polling timeout. If we want a shorter timeout for other requests, we can control that using the cancellation token.

I went ahead and updated the timeout and the catch here. Question though: Is the Task.Delay even necessary at this point?

johnml1135 · 2023-09-29T21:39:54Z

Task delay is still necessary if it increments very quickly. I have seen dozens if not, hundreds of requests per second on SMT training. There should be a max rate for updating. Get Outlook for iOS<https://aka.ms/o0ukef>

…

________________________________ From: Eli C. Lowry ***@***.***> Sent: Friday, September 29, 2023 4:14:14 PM To: sillsdev/serval ***@***.***> Cc: John Lambert ***@***.***>; Mention ***@***.***> Subject: Re: [sillsdev/serval] Revert to long polling in the ServalClientHelper. Fixes #98. (PR #143) tests/Serval.E2ETests/ServalClientHelper.cs line 132 at r1<https://reviewable.io/reviews/sillsdev/serval/143#-NfM2tV97d8QAMr7n14w:-NfXZn6q-aiQ4ZP8BcGh:b-8e2gn> (raw file<https://github.com/sillsdev/serval/blob/e0829dd3952465565df428a6455d4aadd088fbda/tests/Serval.E2ETests/ServalClientHelper.cs#L132>): Previously, ddaspit (Damien Daspit) wrote… Yes, we should set the request timeout for HttpClient to something higher than the long polling timeout. If we want a shorter timeout for other requests, we can control that using the cancellation token. I went ahead and updated the timeout and the catch here. Question though: Is the Task.Delay even necessary at this point? — Reply to this email directly, view it on GitHub<#143 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ADIY5NFKDXSE3H5VTBHG5DLX44TZNANCNFSM6AAAAAA5IJHRUI>. You are receiving this because you were mentioned.Message ID: ***@***.***>

Enkidu93 · 2023-09-30T14:52:14Z

Task delay is still necessary if it increments very quickly. I have seen dozens if not, hundreds of requests per second on SMT training. There should be a max rate for updating. Get Outlook for iOShttps://aka.ms/o0ukef
…
________________________________ From: Eli C. Lowry @.> Sent: Friday, September 29, 2023 4:14:14 PM To: sillsdev/serval @.> Cc: John Lambert @.>; Mention @.> Subject: Re: [sillsdev/serval] Revert to long polling in the ServalClientHelper. Fixes #98. (PR #143) tests/Serval.E2ETests/ServalClientHelper.cs line 132 at r1https://reviewable.io/reviews/sillsdev/serval/143#-NfM2tV97d8QAMr7n14w:-NfXZn6q-aiQ4ZP8BcGh:b-8e2gn (raw file

serval/tests/Serval.E2ETests/ServalClientHelper.cs

Line 132 in e0829dd

catch

): Previously, ddaspit (Damien Daspit) wrote… Yes, we should set the request timeout for HttpClient to something higher than the long polling timeout. If we want a shorter timeout for other requests, we can control that using the cancellation token. I went ahead and updated the timeout and the catch here. Question though: Is the Task.Delay even necessary at this point? — Reply to this email directly, view it on GitHub<#143 (comment)>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADIY5NFKDXSE3H5VTBHG5DLX44TZNANCNFSM6AAAAAA5IJHRUI. You are receiving this because you were mentioned.Message ID: @.***>

OK, great! Can you approve this request? Or is there something left to do?

johnml1135

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ddaspit)

johnml1135

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ddaspit)

ddaspit

I agree that we update the status too much for SMT training. Do we have an issue for that? We should throttle those updates internally.

Reviewable status: complete! all files reviewed, all discussions resolved

Revert to long polling in the ServalClientHelper. Fixes #98.

e0829dd

Enkidu93 requested review from ddaspit and johnml1135 September 26, 2023 20:24

johnml1135 requested changes Sep 27, 2023

View reviewed changes

Catch exception only on proper timeout

dc70c29

ddaspit requested changes Sep 27, 2023

View reviewed changes

johnml1135 reviewed Sep 27, 2023

View reviewed changes

Fix revision update

163b89f

ddaspit approved these changes Sep 27, 2023

View reviewed changes

Enkidu93 force-pushed the revert_to_long_polling_#98 branch from 163b89f to 027d4f1 Compare September 28, 2023 13:11

Enkidu93 added 3 commits September 28, 2023 09:20

Revert to long polling in the ServalClientHelper. Fixes #98.

c55a48b

Catch exception only on proper timeout

af8221d

Fix revision update

ca8d240

Enkidu93 force-pushed the revert_to_long_polling_#98 branch from 027d4f1 to ca8d240 Compare September 28, 2023 13:20

johnml1135 requested changes Sep 28, 2023

View reviewed changes

johnml1135 reviewed Sep 28, 2023

View reviewed changes

ddaspit reviewed Sep 28, 2023

View reviewed changes

Merge branch 'revert_to_long_polling_#98' of https://github.com/sills…

9e3844f

…dev/serval into revert_to_long_polling_#98

Enkidu93 commented Sep 28, 2023

View reviewed changes

ddaspit reviewed Sep 28, 2023

View reviewed changes

johnml1135 requested changes Sep 29, 2023

View reviewed changes

ddaspit reviewed Sep 29, 2023

View reviewed changes

Update http client configuration and corresponding catch statement

63746aa

johnml1135 approved these changes Oct 2, 2023

View reviewed changes

johnml1135 merged commit bad1b9a into main Oct 2, 2023
1 of 2 checks passed

ddaspit reviewed Oct 2, 2023

View reviewed changes

johnml1135 mentioned this pull request Oct 2, 2023

Internal throttle of SMT updates sillsdev/machine#100

Closed

Enkidu93 deleted the revert_to_long_polling_#98 branch October 10, 2023 17:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert to long polling in the ServalClientHelper. Fixes #98. #143

Revert to long polling in the ServalClientHelper. Fixes #98. #143

Enkidu93 commented Sep 26, 2023 •

edited by ddaspit

johnml1135 commented Sep 27, 2023

johnml1135 left a comment

Enkidu93 commented Sep 27, 2023

johnml1135 commented Sep 27, 2023

ddaspit left a comment

johnml1135 left a comment

ddaspit left a comment

Enkidu93 commented Sep 28, 2023

johnml1135 commented Sep 28, 2023

johnml1135 left a comment

johnml1135 commented Sep 28, 2023

johnml1135 left a comment

ddaspit left a comment

Enkidu93 commented Sep 28, 2023

Enkidu93 left a comment

ddaspit left a comment

johnml1135 commented Sep 29, 2023

johnml1135 commented Sep 29, 2023

johnml1135 left a comment

ddaspit left a comment

Enkidu93 commented Sep 29, 2023

johnml1135 commented Sep 29, 2023 via email

Enkidu93 commented Sep 30, 2023

johnml1135 left a comment

johnml1135 left a comment

ddaspit left a comment

Revert to long polling in the ServalClientHelper. Fixes #98. #143

Revert to long polling in the ServalClientHelper. Fixes #98. #143

Conversation

Enkidu93 commented Sep 26, 2023 • edited by ddaspit

johnml1135 commented Sep 27, 2023

johnml1135 left a comment

Choose a reason for hiding this comment

Enkidu93 commented Sep 27, 2023

johnml1135 commented Sep 27, 2023

ddaspit left a comment

Choose a reason for hiding this comment

johnml1135 left a comment

Choose a reason for hiding this comment

ddaspit left a comment

Choose a reason for hiding this comment

Enkidu93 commented Sep 28, 2023

johnml1135 commented Sep 28, 2023

johnml1135 left a comment

Choose a reason for hiding this comment

johnml1135 commented Sep 28, 2023

johnml1135 left a comment

Choose a reason for hiding this comment

ddaspit left a comment

Choose a reason for hiding this comment

Enkidu93 commented Sep 28, 2023

Enkidu93 left a comment

Choose a reason for hiding this comment

ddaspit left a comment

Choose a reason for hiding this comment

johnml1135 commented Sep 29, 2023

johnml1135 commented Sep 29, 2023

johnml1135 left a comment

Choose a reason for hiding this comment

ddaspit left a comment

Choose a reason for hiding this comment

Enkidu93 commented Sep 29, 2023

johnml1135 commented Sep 29, 2023 via email

Enkidu93 commented Sep 30, 2023

johnml1135 left a comment

Choose a reason for hiding this comment

johnml1135 left a comment

Choose a reason for hiding this comment

ddaspit left a comment

Choose a reason for hiding this comment

Enkidu93 commented Sep 26, 2023 •

edited by ddaspit