Skip to content

tikv/txn: add the method BatchGet (#108)#109

Merged
disksing merged 9 commits intotikv:v2from
gotoxu:batch_get
Jun 22, 2021
Merged

tikv/txn: add the method BatchGet (#108)#109
disksing merged 9 commits intotikv:v2from
gotoxu:batch_get

Conversation

@gotoxu
Copy link
Contributor

@gotoxu gotoxu commented Jun 21, 2021

Signed-off-by: Xu Qiaolun jamesxql@gmail.com

fix issue: #108

Signed-off-by: Xu Qiaolun <jamesxql@gmail.com>
@disksing
Copy link
Collaborator

Hi @gotoxu , would you like to add some test?

Signed-off-by: Xu Qiaolun <jamesxql@gmail.com>
@gotoxu
Copy link
Contributor Author

gotoxu commented Jun 22, 2021

Hi @disksing , I have added a test case for BufferBatchGetter!

@disksing
Copy link
Collaborator

Hi @gotoxu , please add license headers to each new file. It looks that the code is mainly copied from tidb project, you will need to keep original TiDB license and a reference link to original file. Just like I have done in #110

@disksing
Copy link
Collaborator

Another question, do you have plan to update TiDB code to use the new method?

return nil
}

func (s *mockBatchGetterStore) Iter(k []byte, upperBound []byte) (unionstore.Iterator, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Could we remove this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is implemented because the parameters of the NewUnionStore method need to implement the uSnapshot interface:

// uSnapshot defines the interface for the snapshot fetched from KV store.
type uSnapshot interface {
	// Get gets the value for key k from kv store.
	// If corresponding kv pair does not exist, it returns nil and ErrNotExist.
	Get(ctx context.Context, k []byte) ([]byte, error)
	// Iter creates an Iterator positioned on the first entry that k <= entry's key.
	// If such entry is not found, it returns an invalid Iterator with no error.
	// It yields only keys that < upperBound. If upperBound is nil, it means the upperBound is unbounded.
	// The Iterator must be Closed after use.
	Iter(k []byte, upperBound []byte) (Iterator, error)

	// IterReverse creates a reversed Iterator positioned on the first entry which key is less than k.
	// The returned iterator will iterate from greater key to smaller key.
	// If k is nil, the returned iterator will be positioned at the last key.
	// TODO: Add lower bound limit
	IterReverse(k []byte) (Iterator, error)
}

In fact, I just need to create a MemDB object. Can we consider exposing the newMemDB method?

Copy link
Member

Choose a reason for hiding this comment

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

How about using mockBatchGetterStore as the first argument for the function NewBufferBatchGetter since the mockBatchGetterStore already implement the interface BatchBufferGetter, and also this would make the test easier to understand(the reviewer needn't understand what is UnionStore which could not be related to this test).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a great idea @AndreMouche

@gotoxu
Copy link
Contributor Author

gotoxu commented Jun 22, 2021

Another question, do you have plan to update TiDB code to use the new method?

We do not use TiDB, we just use TiKV as the storage layer of our entire system, so we need a separate Go client project.

@disksing
Copy link
Collaborator

Another question, do you have plan to update TiDB code to use the new method?

We do not use TiDB, we just use TiKV as the storage layer of our entire system, so we need a separate Go client project.

Got it. We will create a linked issue at TiDB side and handle it later.

@disksing
Copy link
Collaborator

LGTM. Please help to take another look. /cc @AndreMouche

Copy link
Member

@AndreMouche AndreMouche left a comment

Choose a reason for hiding this comment

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

Rest LGTM

@disksing
Copy link
Collaborator

Sorry, @gotoxu I have missed one problem. As we are working on replacing pingcap/check with testify in all tests. Please rewrite the test using testify, thanks! You can find some examples in PRs linked to this issue: #97

@gotoxu
Copy link
Contributor Author

gotoxu commented Jun 22, 2021

Sorry, @gotoxu I have missed one problem. As we are working on replacing pingcap/check with testify in all tests. Please rewrite the test using testify, thanks! You can find some examples in PRs linked to this issue: #97

done!

@disksing
Copy link
Collaborator

Thanks @gotoxu , I'll update your branch before merging.

@disksing disksing merged commit 776abcd into tikv:v2 Jun 22, 2021
tiancaiamao pushed a commit to tiancaiamao/client-go that referenced this pull request Jul 8, 2021
Signed-off-by: Xu Qiaolun <jamesxql@gmail.com>

Co-authored-by: disksing <i@disksing.com>
Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
daimashusheng pushed a commit to daimashusheng/client-go that referenced this pull request Sep 2, 2021
Signed-off-by: Xu Qiaolun <jamesxql@gmail.com>

Co-authored-by: disksing <i@disksing.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants