JanusGraph / janusgraph-foundationdb

FoundationDB storage adapter for JanusGraph
Other
53 stars 18 forks source link

Add range and partition support on FoundationDB adapter for batch processing #48

Closed ruweih closed 3 years ago

linux-foundation-easycla[bot] commented 3 years ago

CLA Signed

The committers are authorized under a signed CLA.

pluradj commented 3 years ago

@ruweih Regarding the CLA check, please fix your commits so that you sign them with your IBM email address instead of your Gmail address. Thanks.

porunov commented 3 years ago

@ruweih Thank you for the contribution! You have 4 commits in this PR and two of them are not signed properly. If you squash and sign you commit, EasyCLA will pass. Let me know if you have troubles squashing those commits and I will help

ruweih commented 3 years ago

@ruweih Regarding the CLA check, please fix your commits so that you sign them with your IBM email address instead of your Gmail address. Thanks.

How could we change the existing commits?

ruweih commented 3 years ago

@ruweih Thank you for the contribution! You have 4 commits in this PR and two of them are not signed properly. If you squash and sign you commit, EasyCLA will pass. Let me know if you have troubles squashing those commits and I will help

Thanks. I used git desktop to merge from master, which for some reason does not signed it properly, how do I change the signature of those existing commits?

porunov commented 3 years ago

Thanks. I used git desktop to merge from master, which for some reason does not signed it properly, how do I change the signature of those existing commits?

@ruweih There are different ways to do it. In you situation, you have some merge commits. You could use git rebase to pick one commit and squash other commits in it ( i.e. git rebase -i 4408551d42c3fafeeee70f103f28fefb409022d3) but in your situation you will also need to resolve conflicts and then do git rebase --continue after the resolutions.

In some cases you know that you PR changes are good and you don't want to worry about past commits and resolve merge commits conflicts. If so, you can merge you branch into the target branch with a squash and then reset you feature branch from target branch, push it and then reset you local target branch from origin (or anything else to remove you squashed merge commit). So, basically, to simplify current squash process I would basically do something like:

git checkout master
git merge --squash rangekeys-ruweih
git commit -m "Add range and partition support on FoundationDB adapter for batch processing" -s
git checkout rangekeys-ruweih
git reset --hard master
git push -f
git checkout master
git reset --hard origin/master

That said, you can use any other technique to squash your commits but make sure to sign them properly with -s parameter.

Also, in case you have incorrect commit author name / email, you can fix it with: git commit --amend --author="Author Name <author@email.org>" -s

but make sure to also use the correct name / email for your commits in case you have a wrong email / name set in the repository. You can change your local repo email / name with something like:

git config user.name "Author Name"
git config user.email "author@email.org"
ruweih commented 3 years ago

@porunov Thanks for the help. All checks been passed after the fix.

@rngcntr The updated PR removed the need for own async iterator, which should address your previous comment. Please review again and clean the change request.

ruweih commented 3 years ago

@rngcntr Seems I don't have the write permission to merge it. Could you or someone else please merge the PR in your convenience if there is no more issue need to be addressed?

porunov commented 3 years ago

@ruweih - two committer approvals OR one committer approval + 1 week for lazy consensus is needed to merge the PR

theZiki commented 3 years ago

Can somebody merge this, I am waiting for this fix for one year :-)

rngcntr commented 3 years ago

@theZiki We need either another committer to approve the PR or wait another four days until we are good to go. Although you are not currently a committer, you are welcome to review the PR, making the review process easier for other committers. (Just in general, probably not as relevant for this relatively small change)

davisdk commented 3 years ago

I've also got my eyes on this one. :] Any chance we can get it merged shortly? Thanks!

rngcntr commented 3 years ago

@davisdk Thanks for reminding me. Since there were no remarks by others, we can merge this PR now.