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

Adds timeouts for all kinds of statements. Power to the clients! #4824

Merged
merged 1 commit into from
Apr 22, 2019
Merged

Adds timeouts for all kinds of statements. Power to the clients! #4824

merged 1 commit into from
Apr 22, 2019

Conversation

rafael
Copy link
Member

@rafael rafael commented Apr 18, 2019

Description

  • When we originally added support for this timeout, we only did it for selects. The rationale is that for queries that had side effect, timing them out could be miss-leading, because it might have succeeded.
  • However, this has caused tons of confusion in our clients. They already set timeouts for insert/delete/updates and they are expecting this timeouts to be propagates.
  • The following PR, adds supports for this.

Tests

  • Added similar tests to the ones existing for select.
  • Integration test missing.

@rafael rafael requested a review from sougou as a code owner April 18, 2019 23:31
@rafael rafael force-pushed the adds-timeouts-for-all-queries branch from f2e11ff to 68950e9 Compare April 18, 2019 23:31
@rafael
Adds timeouts for all kinds of statements. Power to the clients!
fffea26 
Signed-off-by: Rafael Chacon <rafael@slack-corp.com>
@rafael rafael force-pushed the adds-timeouts-for-all-queries branch from 68950e9 to fffea26 Compare April 18, 2019 23:42
@zmagg
Copy link
Contributor

zmagg commented Apr 19, 2019

Yay. This seems pretty straightforward to me. Might be nice to add tests for one of the other non scatter cases too, but seems fine.

sougou
sougou approved these changes Apr 22, 2019
View reviewed changes
Copy link
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

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

I've been slowly working towards unifying the select and dml plan building. With the SBR deprecation, we can be more permissive with cross shard plans, joins, etc.

And we'll get to unify all this code that's beginning to fragment.

@sougou sougou merged commit 21167f1 into vitessio:master Apr 22, 2019
@rafael rafael deleted the adds-timeouts-for-all-queries branch April 22, 2019 16:22
@setassociative setassociative mentioned this pull request Apr 26, 2019
Merged
setassociative added a commit to tinyspeck/vitess that referenced this pull request Apr 26, 2019
@setassociative
Merge upstream 2019-04-25 (#130)
3489b15 
It's been busy and exciting in vitess upstream. Some changes that felt worth calling out as they could impact assumptions about behavior:

- vitessio#4832 — this adds a timeout that is 3x the `healthCheckInterval` which at which point the tablet will stop reporting as healthy
- vitessio#4814 — this changes when command line args are used by vttablet
- vitessio#4811 — a planned reparent now sets `super_read_only`
- vitessio#4805 — behavior coming back from a failed backup is different

**Slack changes**  
        @demmer vitessio#4827 Correct suppress logging for begin...commit in autocommit
        @rafael vitessio#4824 adds timeouts for all statements
        @demmer vitessio#4826 adds logging of stack traces as opt-in
        @demmer vitessio#4819 remove begin/commit logs from autocommit txns
        @demmer vitessio#4796 improves support for vtexplain for begin/dml/dml/commit txns

**Non slack changes**  
        vitessio#4839 Improve behavior with reference table routing & vreplication
        vitessio#4833 Support query routing given there could be multiple targets for a table
        vitessio#4832 tablets get new health check behavior (health checks time out)
        vitessio#4837 refresh bug around row streamer handling creds
        vitessio#4830 apply a default value to the db_name attribute in local_metadata
        vitessio#4785 SHOW SCHEMAS aliased to SHOW DATABASES
        vitessio#4829 add experimental support for split clone & vertical split clone via vreplication
        vitessio#4822 do not normalize in order by
        vitessio#4791 Adds vreplication row streaming support
        vitessio#4814 command line flags now used regardless of management mode
        vitessio#4811 vttablet sets super_read_only during planned reparent
        vitessio#4803 xtrabackup testing
        vitessio#4727 support multiple vttablets running against one mysql instance
        vitessio#4746 TopoCat can now produce JSON output
        vitessio#4805 mysqld is now restarted after a failed backup
        vitessio#4685 macos bootstrap bug
        vitessio#4874 ZK opts and java land
        vitessio#4695 Adds support for xtrabackup
        vitessio#4794 fixes build failures
        vitessio#4725 changes mysql8.0 start args
        vitessio#4736 introduce new states to support vreplication
        vitessio#4788 expands orc error

**Docs etc**  
        vitessio#4831 lint
        vitessio#4827 docs
        vitessio#4816 const declaration cleanup
        vitessio#4820 const declaration cleanup
        vitessio#4825 docs
        vitessio#4818 docs
        vitessio#4809 docs
        vitessio#4812 moves consts around
        vitessio#4813 docs
        vitessio#4808 docs
        vitessio#4800 docs
        vitessio#4795 docs
This was referenced Jun 5, 2019
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sougou sougou sougou approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rafael @zmagg @sougou