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(table): add retries for insert partial failures #589

Merged
merged 9 commits into from Apr 13, 2020
Merged

Conversation

callmehiphop
Copy link
Contributor

@callmehiphop callmehiphop commented Dec 9, 2019

  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 9, 2019
@callmehiphop callmehiphop added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Dec 9, 2019
src/table.ts Outdated Show resolved Hide resolved
@callmehiphop callmehiphop requested review from tswast, shollyman and a team December 10, 2019 18:14
@bcoe
Copy link
Contributor

bcoe commented Dec 12, 2019

@callmehiphop is there an issue or internal document related to this, curious to have a bitt of background.

@callmehiphop
Copy link
Contributor Author

@bcoe there is an internal issue, but I couldn't tell you where to find it. @steffnay could probably help here

@meredithslota
Copy link
Contributor

meredithslota commented Dec 17, 2019

@bcoe b/145598828 is the internal issue.

Copy link
Contributor

@shollyman shollyman left a comment

Choose a reason for hiding this comment

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

This seems like a reasonable approach (finite retry when there are entries in the structured error response from invoking tabledata.insertAll).

@meredithslota meredithslota requested review from bcoe and removed request for tswast and bcoe January 21, 2020 18:57
@zamnuts zamnuts self-assigned this Mar 1, 2020
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Mar 9, 2020
@codecov
Copy link

codecov bot commented Mar 9, 2020

Codecov Report

Merging #589 into master will increase coverage by 0.02%.
The diff coverage is 98.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #589      +/-   ##
==========================================
+ Coverage   95.10%   95.13%   +0.02%     
==========================================
  Files           7        7              
  Lines        6214     6290      +76     
  Branches      397      388       -9     
==========================================
+ Hits         5910     5984      +74     
- Misses        304      306       +2     
Impacted Files Coverage Δ
src/table.ts 99.91% <98.57%> (-0.09%) ⬇️

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 7984e0b...f8c850b. Read the comment docs.

src/table.ts Show resolved Hide resolved
@zamnuts
Copy link
Contributor

zamnuts commented Mar 9, 2020

@googlebot I consent

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Mar 9, 2020
@zamnuts zamnuts requested a review from shollyman March 9, 2020 11:32
@JustinBeckwith JustinBeckwith added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 9, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 9, 2020
src/table.ts Outdated Show resolved Hide resolved
src/table.ts Outdated Show resolved Hide resolved
test/table.ts Outdated Show resolved Hide resolved
test/table.ts Outdated Show resolved Hide resolved
test/table.ts Outdated Show resolved Hide resolved
@zamnuts
Copy link
Contributor

zamnuts commented Mar 23, 2020

Waiting on changes from #647 to resolve codecov decrease and to remove test TODO

@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 31, 2020
src/table.ts Outdated Show resolved Hide resolved
test/table.ts Outdated Show resolved Hide resolved
test/table.ts Show resolved Hide resolved
test/table.ts Show resolved Hide resolved
@JustinBeckwith JustinBeckwith self-requested a review March 31, 2020 15:56
@JustinBeckwith JustinBeckwith dismissed their stale review March 31, 2020 15:57

I defer my review to Steffany and Ben

zamnuts added a commit that referenced this pull request Apr 6, 2020
@zamnuts zamnuts added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 6, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 6, 2020
@zamnuts zamnuts requested a review from steffnay April 6, 2020 05:48
@zamnuts zamnuts added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 11, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 12, 2020
@zamnuts zamnuts added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 12, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 12, 2020
@zamnuts zamnuts added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 13, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 13, 2020
@zamnuts zamnuts merged commit b8639c2 into master Apr 13, 2020
@stephenplusplus stephenplusplus deleted the insert-retries branch April 13, 2020 20:08
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. do not merge Indicates a pull request not ready for merge, due to either quality or timing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants