Skip to content

tabletserver: retry streaming query on conn error#1539

Merged
sougou merged 9 commits intomasterfrom
sugufix
Mar 3, 2016
Merged

tabletserver: retry streaming query on conn error#1539
sougou merged 9 commits intomasterfrom
sugufix

Conversation

@sougou
Copy link
Copy Markdown
Contributor

@sougou sougou commented Mar 2, 2016

@erzel
Streaming queries have recently become killable due to context
changes. So, a connection could potentially be dead due to a
previous query kill. So, we should retry streaming queries also
if we get a connection error.

Review on Reviewable

Streaming queries have recently become killable due to context
changes. So, a connection could potentially be dead due to a
previous query kill. So, we should retry streaming queries also
if we get a connection error.
@michael-berlin
Copy link
Copy Markdown
Contributor

Sugu, you accidentally removed the execution bits from test.go in your first commit.

Can you please restore it?

This is probably the easiest way:

# Make sure your branch is clear and you don't have pending changes.
git update-index --chmod=+x test.go
git commit

Thanks!

@erzel
Copy link
Copy Markdown
Contributor

erzel commented Mar 2, 2016

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion.


go/vt/tabletserver/dbconn.go, line 132 [r1] (raw file):
Shouldn't it be dbconn.Stream in the error message? Also, wouldn't a "panic" be better here?


Comments from the review on Reviewable.io

@sougou
Copy link
Copy Markdown
Contributor Author

sougou commented Mar 2, 2016

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.


go/vt/tabletserver/dbconn.go, line 132 [r1] (raw file):
Done.
This used to be a panic until @aaijazi changed this as part of his error cleanup, but I don't remember the reasons any more.


Comments from the review on Reviewable.io

@aaijazi
Copy link
Copy Markdown
Contributor

aaijazi commented Mar 2, 2016

go/vt/tabletserver/dbconn.go, line 132 [r1] (raw file):
I have no idea either. Seems like this PR was when it changed: #906


Comments from the review on Reviewable.io

@erzel
Copy link
Copy Markdown
Contributor

erzel commented Mar 3, 2016

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.


go/vt/tabletserver/dbconn.go, line 132 [r1] (raw file):
IMHO this should be a panic. LGTM otherwise.


Comments from the review on Reviewable.io

@sougou
Copy link
Copy Markdown
Contributor Author

sougou commented Mar 3, 2016

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion.


go/vt/tabletserver/dbconn.go, line 132 [r1] (raw file):
Done.


Comments from the review on Reviewable.io

@sougou
Copy link
Copy Markdown
Contributor Author

sougou commented Mar 3, 2016

LGTM
(proxy-approving for @erzel)

Approved with PullApprove

@sougou
Copy link
Copy Markdown
Contributor Author

sougou commented Mar 3, 2016

Reviewed 1 of 1 files at r2, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from the review on Reviewable.io

@sougou
Copy link
Copy Markdown
Contributor Author

sougou commented Mar 3, 2016

Review status: all files reviewed at latest revision, 1 unresolved discussion.


go/vt/tabletserver/dbconn.go, line 132 [r1] (raw file):
Done.


Comments from the review on Reviewable.io

@sougou
Copy link
Copy Markdown
Contributor Author

sougou commented Mar 3, 2016

Review status: all files reviewed at latest revision, all discussions resolved.


go/vt/tabletserver/dbconn.go, line 132 [r1] (raw file):
Looks like anyone who said anything has to approve. So, I'm going to override knowing that we're all in agreement.


Comments from the review on Reviewable.io

@aaijazi
Copy link
Copy Markdown
Contributor

aaijazi commented Mar 3, 2016

@sougou
Copy link
Copy Markdown
Contributor Author

sougou commented Mar 3, 2016

LGTM
again... (apologies for the spam)

Approved with PullApprove

sougou added a commit that referenced this pull request Mar 3, 2016
tabletserver: retry streaming query on conn error
@sougou sougou merged commit 44c6b26 into master Mar 3, 2016
@michael-berlin
Copy link
Copy Markdown
Contributor

I think one LGTM is enough.

If I understood @enisoc correctly, you could also just merge changes yourself since you're an admin of our repository.

@sougou sougou deleted the sugufix branch March 4, 2016 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants