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

fix: parse the response body message to determine retry backoff #249

Merged
merged 3 commits into from
Jul 16, 2018

Conversation

nolanmar511
Copy link
Contributor

This fixes #245.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 11, 2018
@@ -637,7 +637,9 @@ describe('Profiler', () => {
.onCall(0)
.callsArgWith(1, undefined, undefined, {
statusCode: 409,
body: {error: {details: [{retryDelay: '50s'}]}}
body: {
message: 'action throttled, backoff for 50s',

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

// The response currently does not have field containing the retry duration.
// As a work-around, response body's message is parsed to get the backoff
// duration.
// TODO: Remove this work-around and get the retry delay from

This comment was marked as spam.

This comment was marked as spam.

@codecov
Copy link

codecov bot commented Jul 12, 2018

Codecov Report

Merging #249 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #249      +/-   ##
========================================
+ Coverage   89.94%    90%   +0.05%     
========================================
  Files           6      6              
  Lines         388    390       +2     
  Branches       69     68       -1     
========================================
+ Hits          349    351       +2     
  Misses         39     39
Impacted Files Coverage Δ
ts/src/profiler.ts 95.85% <100%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da64079...0d0cc16. Read the comment docs.

@nolanmar511
Copy link
Contributor Author

PTAL

Copy link
Contributor

@aalexand aalexand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I think it's almost done, just couple comments.

// The response currently does not have field containing the server-specified
// backoff. As a workaround, response body's message is parsed to get the
// backoff.
// TODO: Remove this work-around and get the retry delay from

This comment was marked as spam.

This comment was marked as spam.

// The response currently does not have field containing the server-specified
// backoff. As a workaround, response body's message is parsed to get the
// backoff.
// TODO: Remove this work-around and get the retry delay from

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

*/
export function parseBackoffDuration(backoffStr: string): number|undefined {
const found = backoffStr.match(BACKOFF_MSG_PAT);
if (found && found.length >= 2 && typeof found[1] === 'string') {

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@@ -139,7 +139,7 @@ AS_IF([test "x${HAVE_BUILT_GTEST}" = "xyes"],
GTEST_LIBS='$(top_builddir)/../googletest/lib/libgtest.la'
GTEST_VERSION="${GTEST_MIN_VERSION}"])

# TODO([email protected]) Check the types, structures, and other compiler
# ([email protected]) Check the types, structures, and other compiler

This comment was marked as spam.

This comment was marked as spam.

}


const BACKOFF_MSG_PATH =

This comment was marked as spam.

This comment was marked as spam.

@nolanmar511 nolanmar511 force-pushed the update-common branch 2 times, most recently from cf6ca14 to bb429ae Compare July 13, 2018 17:54
@nolanmar511
Copy link
Contributor Author

PTAL

@aalexand
Copy link
Contributor

@nolanmar511 The CI test failed on node10?

@aalexand
Copy link
Contributor

@nolanmar511 Please merge once node10 CI is passing. (assuming ofrobots@ doesn't have more comments)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Server-specified backoff is broken
4 participants