Skip to content

Issue 648 - avoid excessive array list resizing #682

Merged
robertdale merged 1 commit intoJanusGraph:0.2from
pencal:issue-648
Nov 18, 2017
Merged

Issue 648 - avoid excessive array list resizing #682
robertdale merged 1 commit intoJanusGraph:0.2from
pencal:issue-648

Conversation

@pencal
Copy link
Contributor

@pencal pencal commented Oct 31, 2017

Issue: #648

Signed-off-by: Kim Pang Lei ckplei@gmail.com

Thank you for contributing to JanusGraph.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there an issue associated with this PR? Is it referenced in the commit message?
  • Does your PR body contain #xyz where xyz is the issue number you are trying to resolve?
  • Has your PR been rebased against the latest commit within the target branch (typically master)?
  • Is your initial contribution a single, squashed commit?

For code changes:

  • Have you written and/or updated unit tests to verify your changes?
  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE.txt file, including the main LICENSE.txt file in the root of this repository?
  • If applicable, have you updated the NOTICE.txt file, including the main NOTICE.txt file found in the root of this repository?

For documentation related changes:

  • Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check Travis CI for build issues and submit an update to your PR as soon as possible.

@janusgraph-bot janusgraph-bot added the cla: no This PR is not compliant with the CLA label Oct 31, 2017
@janusgraph-bot
Copy link

Committer of one or more commits is not listed as a CLA signer, either individual or as a member of an organization.

@pluradj
Copy link
Member

pluradj commented Oct 31, 2017

@pencal To get the CLA checker to verify you correctly, you need to update your commit so that the Author: field matches your name and email as on the CLA. Currently it is:

Author: pencal <ckplei@gmail.com>

but it should be

Author: Kim Pang Lei <ckplei@gmail.com>

https://github.com/JanusGraph/janusgraph/blob/master/CONTRIBUTING.md#configure-your-repo-to-match-the-cla

@pencal pencal force-pushed the issue-648 branch 2 times, most recently from 7187c2e to adecc42 Compare October 31, 2017 18:53
@pencal
Copy link
Contributor Author

pencal commented Oct 31, 2017

@pluradj thanks. I submitted the PR initially using my github user name instead of my name. Since then I have fixed it but I didn't commit with --reset-author. Let me give that a try.

@pluradj
Copy link
Member

pluradj commented Oct 31, 2017

latest commit looks correct to me, might need to pull in @mbrukman if the CLA checker doesn't clear up

@pencal
Copy link
Contributor Author

pencal commented Oct 31, 2017

thanks @pluradj I wasn't sure if the check will run for each push. But looks like it would according to the conversation I have in other threads. Or may be someone does have to clear manually first?

public List<EntryList> execute(final BackendTransaction tx) {
int total = 0;
List<EntryList> result = new ArrayList<EntryList>(4);
List<EntryList> result = new ArrayList(Math.min(getLimit(), queries.size()));
Copy link
Member

Choose a reason for hiding this comment

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

Should be typed. e.g. new ArrayList<>

Copy link
Member

Choose a reason for hiding this comment

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

And final.

@pencal pencal force-pushed the issue-648 branch 2 times, most recently from 920a1e2 to b060ed9 Compare October 31, 2017 20:47
@janusgraph-bot janusgraph-bot added cla: yes This PR is compliant with the CLA and removed cla: no This PR is not compliant with the CLA labels Oct 31, 2017
@mbrukman
Copy link
Member

mbrukman commented Oct 31, 2017

The issue is that there's a difference in GitHub in terms of who the "author" and "committer" are (as they could be different people); the CLA checker is very simple: it verifies that both of these are each on the CLA verbatim; in this case, they are:

author: Kim Pang Lei <ckplei@gmail.com>, GitHub: pencal
committer: pencal <ckplei@gmail.com>, GitHub: pencal

which means the first one is OK, and the second one is not. You can see this yourself for the most-recent commit in your git tree via:

$ git show --format=full

or for the full repo via:

$ git log --format=full

Note the Author: and Commit: lines.

In addition, your Signed-off-by line is non-standard, because it doesn't include < and > around the email address. This suggests you did not follow the commands verbatim as suggested in the documentation, because the -s flag to git commit adds the < and >.

You need to copy-paste and run these commands as-is, verbatim, as per the docs:

$ git config user.name "My Name"
$ git config user.email "my-email@example.com"
$ git commit --amend -s --reset-author
$ git push -f

And possibly remove / adjust any config that could be overriding this. Note that the repo-local config should be overriding your global config in this case, so be sure to set both name and email in the repo to take precedence.

If you're still having an issue, we can look into it further, but we haven't had this happen and it was fixed after using these commands, so I'm hopeful this should solve the problem for you.

@mbrukman
Copy link
Member

@pencal – congrats! You now have a green CLA tag on this PR. Please keep doing what you did in this PR going forward, this is the correct course of action.

@pencal pencal force-pushed the issue-648 branch 2 times, most recently from d91843e to c18d725 Compare November 1, 2017 18:24
Copy link
Contributor

@sjudeng sjudeng left a comment

Choose a reason for hiding this comment

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

Is the added test relevant to the array list resizing fix? The new test passes for me even with the original implementation.

@pencal
Copy link
Contributor Author

pencal commented Nov 2, 2017

@sjudeng the added test is more of a regression. That said, I shall add a test case to test out the case where the limit is accounted for.

@robertdale
Copy link
Member

@pencal Can you rebase this on 0.2 and then merge to master on a new PR?

@pencal
Copy link
Contributor Author

pencal commented Nov 16, 2017

@robertdale why is this necessary? This PR is already merging to master and my branch was based off Master right after 0.2 was cut.

@robertdale
Copy link
Member

@pencal We would like to target the lowest maintenance branch and then merge up. You should only need to change the base of this PR to 0.2 [1]. I can handle the merge. Also, understand that JanusGraph 0.2 will be on TinkerPop 3.2.x while JanusGraph 0.3 will be on TinkerPop 3.3.x which are somewhat incompatible and could have different release timelines.

  1. https://help.github.com/articles/changing-the-base-branch-of-a-pull-request/

@pencal
Copy link
Contributor Author

pencal commented Nov 17, 2017

I see. I will rebase, close this PR and open a new one as suggested. Thanks for giving me the context.

@robertdale
Copy link
Member

@pencal No need to close. Just change the base branch as shown in that link!

@pencal pencal changed the base branch from master to 0.2 November 18, 2017 01:24
Signed-off-by: Kim Pang Lei <ckplei@gmail.com>
@pencal
Copy link
Contributor Author

pencal commented Nov 18, 2017

@robertdale done.

@robertdale robertdale merged commit c20a189 into JanusGraph:0.2 Nov 18, 2017
@robertdale robertdale added this to the Release v0.2.1 milestone Nov 18, 2017
@pencal pencal deleted the issue-648 branch November 19, 2017 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This PR is compliant with the CLA kind/enhancement kind/performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants