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.

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

fix(cursor): Last method needs skip empty pages #341

Merged
merged 1 commit into from
Nov 30, 2022

Conversation

dchaofei
Copy link
Contributor

In the same transaction, Detete may cause the last page to be empty. In this case, calling the Last method will return nill instead of the value of the previous page, which is incorrect.

  1. suppose there is a bucket of 1000 elements
  2. the structure diagram at this time
                          +---------------------+                                                                  
                          |     root bucket     |                                                                  
                +---------+---------------------+---------+                                                        
                |                                         |                                                        
                |                                         |                                                        
                |                                         |                                                        
                |                                         |                                                        
                |                                         |                                                        
                |                                         |                                                        
         +--------------------+                 +---------------------------+                                      
         |                    |                 |                           |                                      
 leaf1   |1,2,3,4.......300 |                 |301,302,303.......1000     |  leaf2                               
         |                    |                 |                           |                                      
         +--------------------+                 +---------------------------+           
  1. now calling the Last method should return 1000
  2. ok, now we call the Delete method to delete the last 800 elements
                          +---------------------+                                                                  
                          |     root bucket     |                                                                  
                +---------+---------------------+---------+                                                        
                |                                         |                                                        
                |                                         |                                                        
                |                                         |                                                        
                |                                         |                                                        
                |                                         |                                                        
                |                                         |                                                        
         +----------------+                             +--+                                                       
         |                |                             |  |                                                       
 leaf1   |1,2,3......200 |                             |  |  leaf2 is empty                                       
         |                |                             |  |                                                       
         +----------------+                             +--+                                                       
                                                                                                                     
                                                                                                                   
   
  1. bad thing happened, calling the Last method again returns nil, but I think it should return 200

@dchaofei dchaofei changed the title fix(cursor): Last needs skip empty pages fix(cursor): Last method needs skip empty pages Oct 13, 2022
@ahrtr ahrtr self-requested a review November 13, 2022 05:53
@ahrtr
Copy link
Member

ahrtr commented Nov 13, 2022

Before I have a deep dive into this PR, please answer question below:

  1. Do you see any real issue in production or test environment? Or can you provide a detailed step to reproduce this issue?

@dchaofei
Copy link
Contributor Author

Before I have a deep dive into this PR, please answer question below:

  1. Do you see any real issue in production or test environment? Or can you provide a detailed step to reproduce this issue?

I found this problem while reading bbolt source code.

There is a test case TestCursor_Last_EmptyPages in pr to reproduce this problem

func TestCursor_Last_EmptyPages(t *testing.T) {

@ahrtr
Copy link
Member

ahrtr commented Nov 13, 2022

Thanks for the feedback. Will take a look later.

Recently we added some workflow, so please rebase this PR.

@dchaofei
Copy link
Contributor Author

@ahrtr hi, I need your approval to execute the test workflow

@ahrtr
Copy link
Member

ahrtr commented Nov 14, 2022

Please always rebase PR instead of merging master.

Please also squash the commits.

@dchaofei
Copy link
Contributor Author

ok, it has been changed to rebase

@dchaofei dchaofei force-pushed the fix_cursor_last branch 2 times, most recently from 8dd9538 to c413227 Compare November 14, 2022 06:53
cursor.go Show resolved Hide resolved
@ahrtr
Copy link
Member

ahrtr commented Nov 18, 2022

Confirmed that it's an real issue, and overall looks good to me.

cursor.go Outdated Show resolved Hide resolved
@ahrtr
Copy link
Member

ahrtr commented Nov 18, 2022

Please also add an item something like below into CHANGE,

Fix the "Last" method might return no data due to not skipping the empty pages.

CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: dchaofei <dchaofei@163.com>
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM

Thank you @dchaofei

cc @ptabor PTAL

@ahrtr ahrtr merged commit 89c1e71 into etcd-io:master Nov 30, 2022
@ahrtr ahrtr added this to the 1.3.7 milestone Dec 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants