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

only write vertex key when flag is set or explictly insert vertex #4680

Merged
merged 1 commit into from
Sep 27, 2022
Merged

only write vertex key when flag is set or explictly insert vertex #4680

merged 1 commit into from
Sep 27, 2022

Conversation

critical27
Copy link
Contributor

What type of PR is this?

  • bug
  • feature
  • enhancement

What problem(s) does this PR solve?

Issue(s) number:

Description:

According to PRD, the vertex key should only insert it when the user_vertex_key is turned on or user insert vertex (not tag) explicitly.

How do you solve it?

Because insert vertex/tag use the same code from graphd to storaged, it seems we can only judge if it is insert_vertex by checking the prop_names is empty.

Special notes for your reviewer, ex. impact of this fix, design document, etc:

Checklist:

Tests:

  • Unit test(positive and negative cases)
  • Function test
  • Performance test
  • N/A

Affects:

  • Documentation affected (Please add the label if documentation needs to be modified.)
  • Incompatibility (If it breaks the compatibility, please describe it and add the label.）
  • If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
  • Performance impacted: Consumes more CPU/Memory

Release notes:

Please confirm whether to be reflected in release notes and how to describe:

ex. Fixed the bug .....

@critical27 critical27 added ready-for-testing PR: ready for the CI test ready for review labels Sep 27, 2022
@Sophie-Xie Sophie-Xie merged commit a5bed33 into vesoft-inc:master Sep 27, 2022
@critical27 critical27 deleted the fix branch September 27, 2022 06:25
@critical27 critical27 mentioned this pull request Sep 28, 2022
Merged
11 tasks
@wey-gu wey-gu mentioned this pull request Oct 1, 2022
Closed
critical27 added a commit to critical27/nebula that referenced this pull request Oct 12, 2022
@critical27
Revert "only write vertex key when flag is set or explictly insert ve…
70fc6eb 
…rtex (vesoft-inc#4680)"

This reverts commit a5bed33.
@critical27 critical27 mentioned this pull request Oct 12, 2022
Merged
11 tasks
critical27 added a commit to critical27/nebula that referenced this pull request Oct 12, 2022
@critical27
Revert "only write vertex key when flag is set or explictly insert ve…
496e33f 
…rtex (vesoft-inc#4680)"

This reverts commit a5bed33.
critical27 added a commit that referenced this pull request Oct 13, 2022
@critical27
Use vertex key only when use_vertex_key is on (#4716)
3a2912d 
* Revert "insert vertex key when only vertex and flag is set (#4685)"

This reverts commit fff82a6.

* Revert "only write vertex key when flag is set or explictly insert vertex (#4680)"

This reverts commit a5bed33.

* Revert "fix bug (#4675)"

This reverts commit cccc014.

* Revert "Rollback vertex key (#4629)"

This reverts commit 545137a.

* wtf

* tck format
Sophie-Xie pushed a commit that referenced this pull request Oct 13, 2022
@critical27 @Sophie-Xie
Use vertex key only when use_vertex_key is on (#4716)
dca1984 
* Revert "insert vertex key when only vertex and flag is set (#4685)"

This reverts commit fff82a6.

* Revert "only write vertex key when flag is set or explictly insert vertex (#4680)"

This reverts commit a5bed33.

* Revert "fix bug (#4675)"

This reverts commit cccc014.

* Revert "Rollback vertex key (#4629)"

This reverts commit 545137a.

* wtf

* tck format
Sophie-Xie added a commit that referenced this pull request Oct 13, 2022
@Sophie-Xie @critical27
@SuperYoko @czpmango
Cherry pick 3.3.0 (10.13) (#4720)
8d63052 
* Use vertex key only when use_vertex_key is on (#4716)

* Revert "insert vertex key when only vertex and flag is set (#4685)"

This reverts commit fff82a6.

* Revert "only write vertex key when flag is set or explictly insert vertex (#4680)"

This reverts commit a5bed33.

* Revert "fix bug (#4675)"

This reverts commit cccc014.

* Revert "Rollback vertex key (#4629)"

This reverts commit 545137a.

* wtf

* tck format

* fix job status when all finished (#4717)

* fix job name from zone balance to data balance

* address wenhui's comment

* fix create index if not exists behavior (#4705)

Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com>
Co-authored-by: Doodle <13706157+critical27@users.noreply.github.com>

Co-authored-by: Doodle <13706157+critical27@users.noreply.github.com>
Co-authored-by: Alex Xing <90179377+SuperYoko@users.noreply.github.com>
Co-authored-by: kyle.cao <kyle.cao@vesoft.com>
yixinglu pushed a commit to yixinglu/nebula that referenced this pull request Jan 31, 2023
@nebula-bots @critical27
Use vertex key only when use_vertex_key is on (vesoft-inc#1405)
eba755c 
<!--
Thanks for your contribution!
In order to review PR more efficiently, please add information according to the template.
-->

## What type of PR is this?
- [ ] bug
- [ ] feature
- [X] enhancement

## What problem(s) does this PR solve?
#### Issue(s) number: 

#### Description:
PRD of vertex key changed again... Here is the new version of logic:
1. No matter use_vertex_key is on or off, the query result should be consistent with 3.1.
2. If use_vertex_key is on, we will use the vertex key to detect whether the vertex exists, which is the logic in 3.X version.
3. If use_vertex_key is off, we will prefix scan by vertex to detect whether the vertex exists, **which will bring performance regression**
4. We will insert the vertex only when use_vertex_key is on

> By default, use_vertex_key is off

## How do you solve it?
This PR reverts all related changes before (vesoft-inc#4685 vesoft-inc#4680 vesoft-inc#4675 vesoft-inc#4629), and apply the logic above. 

Related changes:
1. So the upgrade tool is necessary again, so bring it back.
2. The TCK cases keeps same as before vesoft-inc#4629


## Special notes for your reviewer, ex. impact of this fix, design document, etc:

> Maybe review the last commit is enough 


## Checklist:
Tests:
- [X] Unit test(positive and negative cases)
- [ ] Function test
- [ ] Performance test
- [ ] N/A

Affects:
- [X] Documentation affected (Please add the label if documentation needs to be modified.)
- [ ] Incompatibility (If it breaks the compatibility, please describe it and add the label.）
- [ ] If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
- [ ] Performance impacted: Consumes more CPU/Memory


## Release notes:

Please confirm whether to be reflected in release notes and how to describe:
> ex. Fixed the bug .....


Migrated from vesoft-inc#4716

Co-authored-by: Doodle <13706157+critical27@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperYoko SuperYoko SuperYoko approved these changes

@panda-sheep panda-sheep panda-sheep approved these changes

@cangfengzhs cangfengzhs Awaiting requested review from cangfengzhs

@shanlai shanlai Awaiting requested review from shanlai

Assignees
No one assigned
Labels
ready for review ready-for-testing PR: ready for the CI test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@critical27 @panda-sheep @SuperYoko @Sophie-Xie