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.

Sign up for GitHub

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

Jump to bottom

Fix part of #11202: Enable logging and add retry for create task #11209

Merged
merged 6 commits into from
Nov 22, 2020
Merged

Fix part of #11202: Enable logging and add retry for create task #11209

merged 6 commits into from
Nov 22, 2020

Conversation

vojtechjelinek
Copy link
Contributor

Overview

  1. This PR fixes or fixes part of ServiceUnavailable when creating a Google Cloud Task #11202.
  2. This PR does the following: Enable debug logging for retry so that we can see whether the create_task is really repeated and use the default retry object instead of None

Essential Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The linter/Karma presubmit checks have passed locally on your machine.
  • "Allow edits from maintainers" is checked. (See here for instructions on how to enable it.)
    • This lets reviewers restart your CircleCI tests for you.
  • The PR is made from a branch that's not called "develop".

PR Pointers

  • Make sure to follow the instructions for making a code change.
  • Oppiabot will notify you when you don't add a PR_CHANGELOG label. If you are unable to do so, please @-mention a code owner (who will be in the Reviewers list), or ask on Gitter.
  • For what code owners will expect, see the Code Owner's wiki page.
  • Make sure your PR follows conventions in the style guide, otherwise this will lead to review delays.
  • Never force push. If you do, your PR will be closed.

@oppiabot
Copy link

oppiabot bot commented Nov 18, 2020

Assigning @seanlip for the first pass review of this PR. Thanks!

seanlip
seanlip approved these changes Nov 18, 2020
View reviewed changes
Copy link
Member

@seanlip seanlip left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

seanlip
seanlip requested changes Nov 18, 2020
View reviewed changes
core/platform/taskqueue/cloud_taskqueue_services.py
# Note: retry=None means that the default retry arguments in queue.yaml are
# used.
response = CLIENT.create_task(parent, task, retry=None)
# Note: retry=retry.Retry() means that the default retry arguments are used.
Copy link
Member

Choose a reason for hiding this comment

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

Wait a sec -- one question about this. Does this mean that we no longer obey the stuff in queue.yaml? If so, that could be confusing if devs update queue.yaml and there is no effect there. Should we perhaps be setting this info in queue.yaml instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not entirnely sure whether the retry params in queue.yaml really work, since it seems that we have a lot of errors comming from this place, I want to try setting the retry here to see if that helps to reduce the number of errors that we get.

@seanlip seanlip assigned vojtechjelinek and unassigned seanlip Nov 18, 2020
@seanlip
Copy link
Member

seanlip commented Nov 18, 2020

Btw you might also need to update isort.cfg to include api_core as a third-party lib, to fix the imports.

seanlip
seanlip approved these changes Nov 19, 2020
View reviewed changes
Copy link
Member

@seanlip seanlip left a comment

Choose a reason for hiding this comment

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

LGTM -- discussed offline, let's try it, monitor, and see if it works. Thanks!

@seanlip seanlip assigned vojtechjelinek and unassigned seanlip Nov 19, 2020
@seanlip
Copy link
Member

seanlip commented Nov 19, 2020

@DubeySandeep @Hudda PTAL as codeowners. Thanks!

DubeySandeep
DubeySandeep approved these changes Nov 19, 2020
View reviewed changes
Copy link
Member

@DubeySandeep DubeySandeep left a comment

Choose a reason for hiding this comment

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

LGTM for the codeowner file

@oppiabot
Copy link

oppiabot bot commented Nov 19, 2020

Unassigning @DubeySandeep since they have already approved the PR.

Hudda
Hudda approved these changes Nov 20, 2020
View reviewed changes
Copy link
Contributor

@Hudda Hudda left a comment

Choose a reason for hiding this comment

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

LGTM for codeowner file.

@Hudda Hudda removed their assignment Nov 20, 2020
@oppiabot oppiabot bot added the PR: LGTM label Nov 20, 2020
@oppiabot
Copy link

oppiabot bot commented Nov 20, 2020

Hi @vojtechjelinek, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!

@kevintab95 kevintab95 closed this Nov 20, 2020
@kevintab95 kevintab95 reopened this Nov 20, 2020
@vojtechjelinek
Copy link
Contributor Author

@BenHenning PTAL, Sean already approved this but you have changed codeowners

@seanlip
Copy link
Member

seanlip commented Nov 22, 2020

Given the review history, this can be merged -- thanks for the ping @vojtechjelinek!

@seanlip seanlip merged commit 42ff402 into oppia:develop Nov 22, 2020
vojtechjelinek added a commit that referenced this pull request Nov 22, 2020
@vojtechjelinek
Fix part of #11202: Enable logging and add retry for create task (#11209
a8817bd 
)

* Enable logging and add retry for create task

* Fix backend tests

* Fix linting

* Fix import sorting
@vojtechjelinek vojtechjelinek mentioned this pull request Nov 23, 2020
Closed
3 tasks
@vojtechjelinek vojtechjelinek mentioned this pull request Nov 24, 2020
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hudda Hudda Hudda approved these changes

@DubeySandeep DubeySandeep DubeySandeep approved these changes

@seanlip seanlip seanlip approved these changes

@BenHenning BenHenning Awaiting requested review from BenHenning

Assignees

@BenHenning BenHenning

Labels
PR: LGTM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vojtechjelinek @seanlip @DubeySandeep @Hudda @kevintab95 @BenHenning