Skip to content

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

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

Merged
merged 1 commit into from
Jan 27, 2021

Conversation

ruweih
Copy link
Contributor

@ruweih ruweih commented Aug 27, 2020

No description provided.

@janusgraph-bot janusgraph-bot added the cla: external Externally-managed CLA label Aug 27, 2020
Copy link
Contributor

@rngcntr rngcntr left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @ruweih!
Using FoundationDB's async reads is a very nice idea. But actually, I don't really know where getSliceAsync will be used in JanusGraph. Is it used at all?

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 3, 2020

CLA Signed

The committers are authorized under a signed CLA.

@pluradj
Copy link
Member

pluradj commented Dec 4, 2020

@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
Copy link
Member

porunov commented Dec 4, 2020

@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
Copy link
Contributor Author

ruweih commented Dec 4, 2020

@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
Copy link
Contributor Author

ruweih commented Dec 4, 2020

@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
Copy link
Member

porunov commented Dec 4, 2020

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 <[email protected]>" -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 "[email protected]"

@ruweih
Copy link
Contributor Author

ruweih commented Dec 4, 2020

@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.

Copy link
Contributor

@rngcntr rngcntr left a comment

Choose a reason for hiding this comment

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

LGTM

@ruweih
Copy link
Contributor Author

ruweih commented Dec 8, 2020

@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
Copy link
Member

porunov commented Dec 8, 2020

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

@theZiki
Copy link

theZiki commented Dec 10, 2020

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

@rngcntr
Copy link
Contributor

rngcntr commented Dec 10, 2020

@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
Copy link

davisdk commented Jan 26, 2021

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

@rngcntr
Copy link
Contributor

rngcntr commented Jan 27, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: external Externally-managed CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants