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

[Gen4] Fix lookup vindexes with autocommit enabled #12172

Merged
merged 4 commits into from
Feb 3, 2023
Merged

[Gen4] Fix lookup vindexes with autocommit enabled #12172

merged 4 commits into from
Feb 3, 2023

Conversation

arthurschreiber
Copy link
Contributor

@arthurschreiber arthurschreiber commented Jan 26, 2023 •
edited
Loading

Description

The VindexLookup primitive re-implemented some parts of the lookup vindex functionality, but missed to honor the autocommit flag. This pull request changes VindexLookup to re-use existing logic instead of using a separate implementation.

Related Issue(s)

Checklist

  • "Backport to:" labels have been added if this change should be back-ported
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

@arthurschreiber arthurschreiber changed the title Fix autocommit vindexes after savepoint creation WIP: Fix autocommit vindexes after savepoint creation Jan 26, 2023
@arthurschreiber arthurschreiber mentioned this pull request Jan 26, 2023
Closed
@arthurschreiber arthurschreiber changed the title WIP: Fix autocommit vindexes after savepoint creation WIP: [Gen4] Fix lookup vindexes with autocommit enabled Jan 27, 2023
@arthurschreiber arthurschreiber force-pushed the arthur/fix-autocommit-with-savepoints branch from 0434014 to 20f66aa Compare January 27, 2023 20:28
@arthurschreiber arthurschreiber marked this pull request as ready for review January 30, 2023 10:02
@vitess-bot
Copy link
Contributor

vitess-bot bot commented Jan 30, 2023

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

  • Ensure that the Pull Request has a descriptive title.
  • If this is a change that users need to know about, please apply the release notes (needs details) label so that merging is blocked unless the summary release notes document is included.
  • If a test is added or modified, there should be a documentation on top of the test to explain what the expected behavior is what the test does.

If a new flag is being introduced:

  • Is it really necessary to add this flag?
  • Flag names should be clear and intuitive (as far as possible)
  • Help text should be descriptive.
  • Flag names should use dashes (-) as word separators rather than underscores (_).

If a workflow is added or modified:

  • Each item in Jobs should be named in order to mark it as required.
  • If the workflow should be required, the maintainer team should be notified.

Bug fixes

  • There should be at least one unit or end-to-end test.
  • The Pull Request description should include a link to an issue that describes the bug.

Non-trivial changes

  • There should be some code comments as to why things are implemented the way they are.

New/Existing features

  • Should be documented, either by modifying the existing documentation or creating new documentation.
  • New features should have a link to a feature request issue or an RFC that documents the use cases, corner cases and test cases.

Backward compatibility

  • Protobuf changes should be wire-compatible.
  • Changes to _vt tables and RPCs need to be backward compatible.
  • vtctl command output order should be stable and awk-able.
  • RPC changes should be compatible with vitess-operator
  • If a flag is removed, then it should also be removed from VTop, if used there.

@arthurschreiber arthurschreiber force-pushed the arthur/fix-autocommit-with-savepoints branch from 20f66aa to 6c72940 Compare January 30, 2023 10:06
harshit-gangal
harshit-gangal reviewed Jan 30, 2023
View reviewed changes
go/vt/vtgate/vindexes/vindex.go Outdated
Comment on lines 144 to 147
AllowBatch() bool
GetCommitOrder() vtgatepb.CommitOrder
}
Copy link
Member

Choose a reason for hiding this comment

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

All the dead code implementations of AllowBatch and GetCommitOrder from the vindexes should also be removed.

Copy link
Member

Choose a reason for hiding this comment

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

Ignore this comment. We have to move autocommit logic as well from Map function to vindex_lookup engine primitive.

@arthurschreiber arthurschreiber force-pushed the arthur/fix-autocommit-with-savepoints branch from 6b480af to 26d1e0c Compare January 30, 2023 23:59
@arthurschreiber
Copy link
Contributor Author

arthurschreiber commented Jan 30, 2023 •
edited
Loading

@harshit-gangal I took a different approach and simply re-implemented the autocommit logic inside the VindexLookup primitive.

I'm not 100% happy with the code duplication that's going on. It would be nice to refactor this further so that both the old API for lookup vindexes and the new API can share the same code / logic so that any future issues only need to be fixed in one place.

@arthurschreiber
Copy link
Contributor Author

@harshit-gangal I think this is ready for another review. With the latest commit I pushed, all test cases should also be passing now. 🤞

@arthurschreiber arthurschreiber changed the title WIP: [Gen4] Fix lookup vindexes with autocommit enabled [Gen4] Fix lookup vindexes with autocommit enabled Jan 31, 2023
harshit-gangal
harshit-gangal approved these changes Feb 1, 2023
View reviewed changes
Copy link
Member

@harshit-gangal harshit-gangal left a comment

Choose a reason for hiding this comment

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

One comment rest looks good.

@arthurschreiber
test: show that autocommit on lookup vindexes is broken when used a…
0295bf5 
…fter a savepoint is created

Signed-off-by: Arthur Schreiber <arthurschreiber@github.com>
@arthurschreiber
test: rework the test case to show that the problem lies with transac…
0c24336 
…tions

Signed-off-by: Arthur Schreiber <arthurschreiber@github.com>
@arthurschreiber
Correctly handle autocommit in the VIndexLookup primitive.
c7b40ba 
Signed-off-by: Arthur Schreiber <arthurschreiber@github.com>
@arthurschreiber
Copy the logging struct so autocommit queries show up correctly in …
8f925ac 
…`vexplain` output.

Signed-off-by: Arthur Schreiber <arthurschreiber@github.com>
@arthurschreiber arthurschreiber force-pushed the arthur/fix-autocommit-with-savepoints branch from eeb17f6 to 8f925ac Compare February 1, 2023 13:50
@systay systay merged commit ee0670f into vitessio:main Feb 3, 2023
@vitess-bot vitess-bot bot mentioned this pull request Feb 3, 2023
Merged
arthurschreiber added a commit that referenced this pull request Feb 3, 2023
@vitess-bot @arthurschreiber
[release-15.0] [Gen4] Fix lookup vindexes with autocommit enabled (#…
91e7790 
…12172) (#12230)

* test: show that `autocommit` on lookup vindexes is broken when used after a savepoint is created

Signed-off-by: Arthur Schreiber <arthurschreiber@github.com>

* test: rework the test case to show that the problem lies with transactions

Signed-off-by: Arthur Schreiber <arthurschreiber@github.com>

* Correctly handle `autocommit` in the `VIndexLookup` primitive.

Signed-off-by: Arthur Schreiber <arthurschreiber@github.com>

* Copy the `logging` struct so autocommit queries show up correctly in `vexplain` output.

Signed-off-by: Arthur Schreiber <arthurschreiber@github.com>

---------

Signed-off-by: Arthur Schreiber <arthurschreiber@github.com>
Co-authored-by: Arthur Schreiber <arthurschreiber@github.com>
@austenLacy austenLacy mentioned this pull request Apr 26, 2023
Merged
@austenLacy austenLacy mentioned this pull request Aug 1, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harshit-gangal harshit-gangal harshit-gangal approved these changes

@systay systay systay approved these changes

@frouioui frouioui Awaiting requested review from frouioui frouioui is a code owner

@GuptaManan100 GuptaManan100 Awaiting requested review from GuptaManan100 GuptaManan100 is a code owner

Assignees
No one assigned
Labels
Component: Query Serving Type: Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug Report: Lookup vindexes with autocommit are not working correctly with Gen4 planner
3 participants
@arthurschreiber @systay @harshit-gangal