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

Use unsaved in-memory commits during key list re-construction #3486

Merged
merged 2 commits into from
Mar 1, 2022

Conversation

dimas-b
Copy link
Member

@dimas-b dimas-b commented Mar 1, 2022

  • Allow the unsaved in-memory commits built during merge / transplant
    operations to be found during key list re-construction.

  • Skip the in-memory lookup in the common case of non-merge call
    paths to reduce the amount of transient lists (parameters and
    results).

  • Parameterize merge / transplant test cases for full coverage
    of related code.

Fixes #3466

@@ -635,23 +636,77 @@ protected void validateHashExists(OP_CONTEXT ctx, Hash hash) throws ReferenceNot
*/
protected abstract List<CommitLogEntry> fetchPageFromCommitLog(OP_CONTEXT ctx, List<Hash> hashes);

private List<CommitLogEntry> fetchPageFromCommitLog(
Copy link
Member

Choose a reason for hiding this comment

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

Mind renaming this to fetchMultipleFromCommitLog? I think, the term "page" is wrong here (and actually always was - it was just used to fetch a whole page).

Copy link
Member Author

Choose a reason for hiding this comment

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

renamed (in a separate commit for ease of review)

@codecov
Copy link

codecov bot commented Mar 1, 2022

Codecov Report

Merging #3486 (49f3e83) into main (9f0a360) will increase coverage by 0.03%.
The diff coverage is 96.72%.

❗ Current head 49f3e83 differs from pull request most recent head 1a86023. Consider uploading reports for the commit 1a86023 to get more accurate results

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #3486      +/-   ##
============================================
+ Coverage     86.62%   86.66%   +0.03%     
- Complexity     3032     3043      +11     
============================================
  Files           376      376              
  Lines         16377    16411      +34     
  Branches       1207     1213       +6     
============================================
+ Hits          14186    14222      +36     
+ Misses         1746     1745       -1     
+ Partials        445      444       -1     
Flag Coverage Δ
java 87.06% <96.72%> (+0.04%) ⬆️
javascript 82.55% <ø> (ø)
python 83.67% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...nessie/versioned/persist/tx/TxDatabaseAdapter.java 76.25% <ø> (ø)
...d/persist/adapter/spi/AbstractDatabaseAdapter.java 95.69% <96.61%> (+0.66%) ⬆️
...persist/nontx/NonTransactionalDatabaseAdapter.java 86.52% <100.00%> (ø)
...rsioned/persist/tests/AbstractMergeTransplant.java 99.02% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f0a360...1a86023. Read the comment docs.

* Allow the unsaved in-memory commits built during merge / transplant
  operations to be found during key list re-construction.

* Skip the in-memory lookup in the common case of non-merge call
  paths to reduce the amount of transient lists (parameters and
  results).

* Parameterize merge / transplant test cases for full coverage
  of related code.

Fixes projectnessie#3466
@dimas-b dimas-b force-pushed the key-list-on-merge branch from 49f3e83 to 1a86023 Compare March 1, 2022 18:39
@dimas-b dimas-b changed the title WIP: Ideas for key-list reconstruction with partial in-memory lookup Use unsaved in-memory commits during key list re-construction Mar 1, 2022
@dimas-b dimas-b marked this pull request as ready for review March 1, 2022 18:40
@snazy snazy merged commit b57ac21 into projectnessie:main Mar 1, 2022
@dimas-b dimas-b deleted the key-list-on-merge branch March 1, 2022 19:35
BranchName main = BranchName.of("main");
BranchName branch = BranchName.of("branch");
BranchName conflict = BranchName.of("conflict");

databaseAdapter.create(branch, databaseAdapter.hashOnReference(main, Optional.empty()));

Hash[] commits = new Hash[3];
Hash[] commits = new Hash[numCommits];
Copy link
Contributor

@ajantha-bhat ajantha-bhat Mar 2, 2022

Choose a reason for hiding this comment

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

I remember that my PR for same issue got blocked because there was no strict validation. But this PR also don't have any validation but went ahead.
I am not sure I understand the process here.

Copy link
Member

Choose a reason for hiding this comment

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

Your change was complex and changed the algorithm/implementation to generate the key-lists.
Dmitri's change did not change that algorithm.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also checked code coverage in IntelliJ (manually) - all branches in the new if statements were covered :)

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.

Merge fails if more than 20 commits are present on the source branch
3 participants