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

feat: rheakv support scan in reverse #422

Merged
merged 1 commit into from
Apr 26, 2020
Merged

Conversation

shibd
Copy link
Contributor

@shibd shibd commented Apr 19, 2020

Motivation:

It looks that rheakv doesn't support scan in reverse order, we should support it.

#340

Modification:

整个代码改动按照scan接口的相关点,新增reverseScan接口,按照reverse scan的含义实现。

Result:

  1. RheaKVStore接口新增reverseScan方法并实现DefaultRheaKVStore。相关测试也补充完毕。
  2. RawKVStore接口新增reverseScan方法,并实现了内存版本MemoryRawKVStoreRocksRawKVStore相关测试也补充完毕。
  3. 新增ReverseScanRequest并在状态机和方法内实现了相关修改,批量操作也按原本实现做了实现。

@sofastack-bot
Copy link

sofastack-bot bot commented Apr 19, 2020

Hi @shibd, we detect non-English characters in the issue. This comment is an auto translation by @sofastack-robot to help other users to understand this issue.

We encourage you to describe your issue in English which is more friendly to other users.

Motivation:> It looks that rheakv doesn't support scan in reverse order, we should support it. # 340 ### Modification: The entire code changes according to the relevant points of the scan interface, adding the reverseScan interface, according to the reverse scan Meaning realized. ### Result: 1. The RheaKVStore interface adds a reverseScan method and implements DefaultRheaKVStore. Related tests are also completed. 2. The ReverseScan method has been added to the RawKVStore interface, and tests related to the memory versions MemoryRawKVStore and RocksRawKVStore have also been added. 3. Added ReverseScanRequest and implemented relevant modifications in the state machine and method, and batch operations were also implemented according to the original implementation.

@shibd
Copy link
Contributor Author

shibd commented Apr 19, 2020

提交了一版代码,请帮忙review。

CI报错,请问这个问题怎么解决 @killme2008

image

@fengjiachun
Copy link
Contributor

提交了一版代码,请帮忙review。

CI报错,请问这个问题怎么解决 @killme2008

image

https://github.com/sofastack/sofa-jraft/blob/master/CONTRIBUTING.md

we provided a code formatter file, it will formatting automatically your project when during process of building.

代码格式有问题,本地执行一下 mvn clean compile 会自动执行代码格式化

@killme2008
Copy link
Contributor

@shibd Ok, i will look into it.

@shibd
Copy link
Contributor Author

shibd commented Apr 20, 2020

提交了一版代码,请帮忙review。
CI报错,请问这个问题怎么解决 @killme2008
image

https://github.com/sofastack/sofa-jraft/blob/master/CONTRIBUTING.md

we provided a code formatter file, it will formatting automatically your project when during process of building.

代码格式有问题,本地执行一下 mvn clean compile 会自动执行代码格式化

收到,谢谢,已经格式化。有个测试没过,我在本地是可以通过的。

Tests in error: 
  chaosSplittingTest(com.alipay.sofa.jraft.rhea.chaos.ChaosMemoryDBLeaderReadBatchingTest): 


com.alipay.sofa.jraft.rhea.errors.InvalidRegionVersionException: Invalid region version error (region split or merge happened).



Tests run: 190, Failures: 0, Errors: 1, Skipped: 0

@fengjiachun
Copy link
Contributor

@shibd 感谢贡献,整体代码质量比较高,提了几个小意见可以看下

@shibd
Copy link
Contributor Author

shibd commented Apr 20, 2020

@shibd 感谢贡献,整体代码质量比较高,提了几个小意见可以看下

谢谢,review的代码已经全部修复,合并了commit log,请再帮忙看一下有没有问题。

@fengjiachun
Copy link
Contributor

@shibd 上一次 cr 有一些遗漏不好意思,麻烦再看下

@shibd
Copy link
Contributor Author

shibd commented Apr 20, 2020

@shibd 上一次 cr 有一些遗漏不好意思,麻烦再看下

好的,谢谢,是我的问题,我现在改。

@shibd
Copy link
Contributor Author

shibd commented Apr 20, 2020

@fengjiachun 您好,修复了上面提的问题,请麻烦再review一下。

@fengjiachun
Copy link
Contributor

@shibd CLA 签一下?

@shibd
Copy link
Contributor Author

shibd commented Apr 21, 2020

@shibd CLA 签一下?

这个怎么签署呢,不好意思第一次参与开源。

@fengjiachun
Copy link
Contributor

image

这里应该可以点进去?

@shibd
Copy link
Contributor Author

shibd commented Apr 21, 2020

这里应该可以点进去?

点击不了,sofastack-bot/cla这个是不是一个网页,有这个地址吗。

@fengjiachun
Copy link
Contributor

这里应该可以点进去?

点击不了,sofastack-bot/cla这个是不是一个网页,有这个地址吗。

是我们这边出了点问题,稍等,相关人在处理中

@sofastack-bot sofastack-bot bot added the cla:no label Apr 21, 2020
@fengjiachun
Copy link
Contributor

@shibd 再看一下,应该可以签了

@fengjiachun
Copy link
Contributor

image

刷新页面点击 Detail

@shibd
Copy link
Contributor Author

shibd commented Apr 21, 2020

image

@fengjiachun 这里显示签署过了,不过看还有提示要签署

image

@sofastack-bot sofastack-bot bot added cla:yes and removed cla:no labels Apr 21, 2020
@fengjiachun
Copy link
Contributor

image

@fengjiachun 这里显示签署过了,不过看还有提示要签署

image

我们的问题,ok 了已经

@fengjiachun
Copy link
Contributor

@killme2008 你这边还有其他意见吗

@killme2008
Copy link
Contributor

@shibd 提了些意见,请再看下

@shibd
Copy link
Contributor Author

shibd commented Apr 26, 2020

@fengjiachun @killme2008 对上面提到的CR做了一些调整,请看一下

@killme2008
Copy link
Contributor

没有其他问题了,测试有没有覆盖 scan 为空的情况,建议也覆盖下

@shibd
Copy link
Contributor Author

shibd commented Apr 26, 2020

没有其他问题了,测试有没有覆盖 scan 为空的情况,建议也覆盖下

@killme2008 谢谢,CR已经修改,合并了commit log。

@shibd
Copy link
Contributor Author

shibd commented Apr 26, 2020

@fengjiachun 您好,看一下还有没有问题。

@killme2008 killme2008 merged commit 82f618e into sofastack:master Apr 26, 2020
@killme2008
Copy link
Contributor

@shibd Congratulations on your first PR! Thanks a lot.

@fengjiachun
Copy link
Contributor

@shibd 感谢

@shibd
Copy link
Contributor Author

shibd commented Apr 27, 2020

@fengjiachun @killme2008 非常感谢两位大佬细心的CR,如果还有具体issue,可以直接分配给我。我会持续学习jraft实现,希望可以为jraft建设出一份力。

@fengjiachun
Copy link
Contributor

@shibd 感谢代码质量很高的 pr,这是我的邮箱 jiachun.fjc@alibaba-inc.com ,可以钉钉或是微信交流,方便的话,可以把钉钉/微信发到我的邮箱,我来加你

zongtanghu pushed a commit that referenced this pull request Apr 28, 2020
@fengjiachun fengjiachun mentioned this pull request Jun 19, 2020
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants