Skip to content

multishard autocommit should work for bypass routing#8428

Merged
harshit-gangal merged 3 commits intomainfrom
multishard-autocommit-should-work-for-scatter-dmls
Jul 13, 2021
Merged

multishard autocommit should work for bypass routing#8428
harshit-gangal merged 3 commits intomainfrom
multishard-autocommit-should-work-for-scatter-dmls

Conversation

@demmer
Copy link
Member

@demmer demmer commented Jul 7, 2021

Description

Parse and extract the multishard autocommit query directive in the bypass routing plan builder.

We (Slack) noticed that routing a DML to the keyrange target [-], performance timings implied that the multishard autocommit query directive wasn't being honored when routing the query.

This change makes the query directive take effect when DMLs are force routed to a particular shard or keyrange.

Checklist

  • Verified manually in our dev environment
  • Added tests for both the engine and the planner
  • Documentation is not required

Deployment Notes

None

@rafael
Copy link
Member

rafael commented Jul 12, 2021

This change looks good to me. @demmer could you look at the conflicts and push again?

Also, extra pair of eyes from @GuptaManan100 or @systay would be great.

demmer added 3 commits July 12, 2021 15:37
Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
@demmer demmer force-pushed the multishard-autocommit-should-work-for-scatter-dmls branch from d6fb25d to b56b89f Compare July 12, 2021 22:37
@demmer
Copy link
Member Author

demmer commented Jul 12, 2021

I fixed the conflicts and re-pushed. Not sure why all the CI is failing.

@GuptaManan100 GuptaManan100 added Component: Query Serving Type: Enhancement Logical improvement (somewhere between a bug and feature) labels Jul 13, 2021
Copy link
Collaborator

@systay systay left a comment

Choose a reason for hiding this comment

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

LGTM

@systay
Copy link
Collaborator

systay commented Jul 13, 2021

I fixed the conflicts and re-pushed. Not sure why all the CI is failing.

Guessing that was the "pr-labels" check that was failing. Manan added labels to the PR, so nothing to do 👍

@harshit-gangal harshit-gangal merged commit 194f15d into main Jul 13, 2021
@harshit-gangal harshit-gangal deleted the multishard-autocommit-should-work-for-scatter-dmls branch July 13, 2021 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: Query Serving Type: Enhancement Logical improvement (somewhere between a bug and feature)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants