Use BFS approach

[caiwei-ebay]

As discussed in MRESOLVER-228, here is the plan for multiple changes requested recently:

• DFS > BFS - preparation for parallel download
• Skip & Reconcile - avoid unnecessary version resolution (MRESOLVER-228)
• Download descriptors in parallel (MRESOLVER-7)

This PR is for DFS-> BFS.

[caiwei-ebay]

@michael-o @ibabiankou

Please help review.

[michael-o]

@cstamas

[ibabiankou]

@caiwei-ebay Thank you for working on this change! I looked at the change and believe we can do a few improvements.

1. The data required to process dependencies is scattered across DataPool and DependencyNodeTraceContext, I think the part in the pool is not needed.
2. In the current implementation DependencyNodeTraceContext represents all dependencies of a single node, given that down the line we want to handle those dependencies in parallel, we should already make sure one entry in the queue corresponds to one dependency.

If you don't mind giving me write access to the fork, I'd love to collaborate and push some changes with minor improvements such as comments, formatting etc. 😉

[caiwei-ebay]

@ibabiankou

Thanks for your valuable feedbacks!
Added you as admin of my forked repo, please go ahead with your changes.

[ibabiankou]

@caiwei-ebay FYI: I've pushed some changes, but a few of the original comments are still to be addressed. I also want to have a fresh look at the naming and javadoc after that. I'll try to wrap it up tomorrow.

[ibabiankou]

@caiwei-ebay I'd say the PR is ready for review from my perspective. Below are the suggested title and description for the PR. WDYT?

Collect dependencies in breadth-first order

This PR changes dependency processing order from depth-first to breadth-first as a preparation for further improvements (MRESOLVER-228, MRESOLVER-7).

**Key changes**
1. NodeStack.find moved to DefaultDependencyCycle since the NodeStack is replaced with a List in the DependencyProcessingContext.
2. Args.dependencyProcessingQueue contains a DependencyProcessingContext for each dependency that should be collected.

Note: This PR does not solve the MRESOLVER-133, though one could expect so from the titles. The main goal of the ticket is to implement early exit when resolving the version range. The ticket should be updated to reflect that.

@michael-o @cstamas I was thinking about rewriting the history of changes to make review easier, however, the only split that makes sense to me is

1. Use breadth-first order (add the new context and use it) ~95% of the change
2. Move the find method to DefaultDependencyCycle and drop NodeStack

And it doesn't seem to make the review much easier 😕

Given that the change is fairly small and the majority of the diff is due to moving the find and adding context. to access the same things from the new place, do you think it is worth the effort?

[michael-o]

@ibabiankou No need to squash at this stage. Pleease make are resolved discussions as such. Waiting for @caiwei-ebay Confirmation that this is ready to review.

[ibabiankou]

Pleease make are resolved discussions as such.

Unfortunately, I can not do that. Looks like I don't have some permissions or something.

[caiwei-ebay]

@michael-o

All the changes @ibabiankou made are good to me. Thanks @ibabiankou!
Please go ahead for the next process.

[ibabiankou]

LGTM 🎉😊

[michael-o]

Thanks you guys, I will pick this up by the end of the week.

[michael-o]

First and foremost question: Does this implement MRESOLVER-133?

[michael-o]

• Guys, can you verbally express what you change actually does?
• Does it incur any resolution result changes? If not, can this be tested some how?

[caiwei-ebay]

@michael-o @ibabiankou
This PR does not solve the MRESOLVER-133, though one could expect so from the titles. The main goal of this PR is to implement pure BFS approach which is the prerequisite for:

• implement early exit when resolving the version range. MRESOLVER-133
• implement a skip approach to avoid unnecessary version resolution. MRESOLVER-228
• implement parallel downloadning, MRESOLVER-7

[caiwei-ebay]

@michael-o
Guys, can you verbally express what you change actually does?

A: This PR is pure BFS approach. Just a change from DFS -> BFS for future PRs related MRESOLVER-133, MRESOLVER-228 and MRESOLVER-7.

With DFS, maven resolves all nodes to compute the dependency tree.
With this BFS solution, it still resolves all nodes to compute the dependency tree.

Later, I would like to submit a PR to skip certain nodes resolving to improve performance.

Does it incur any resolution result changes? If not, can this be tested some how?

A: Nope, it won't introduce any resolution result changes. No tests broken.
Regarding the test, I have dryrun large number of apps in our company by comparing dependency tree and dependency list, no issues found. Not sure if this is the right test method your are expecting.

[cstamas]

pls incorporate latest master changes (whether rebase+squash+force push or just merge)

[caiwei-ebay]

pls incorporate latest master changes (whether rebase+squash+force push or just merge)

@cstamas @michael-o @ibabiankou
Rebased & squashed commits.

[caiwei-ebay]

@cstamas

Looks like the pipeline has broken since 2807538, could you please fix the issue in master branch?

[caiwei-ebay]

@michael-o @cstamas

Saw the master branch has been fixed already. Just rebased the code.
Please let me know if there is any else I can help.

After this PR merged, I would submit PR for below JIRA:

• Skip & Reconcile - avoid unnecessary version resolution (MRESOLVER-228)

Then I think @ibabiankou would submit PR for:

• Download descriptors in parallel (MRESOLVER-7)

[cstamas]

Tested locally. This is what I did (used Java 11 Temurin for all, on Linux):

• built and installed locally maven master branch (0be5e406d78062b56b32644fefce2642f5eab650) -- this is REFERENCE (uses resolver 1.7.3)
• built locally this PR (resolver-bfs)
• built locally and installed this maven PR [Experiment] Maven w/ transport-http maven#635 -- produces maven using resolver-1.8.0-SNAPSHOT-BFS (also uses transport-http instead of wagon, but this is irrelevant now)

Then I went back to maven master (0be5e406d78062b56b32644fefce2642f5eab650) and built it with two contenders, always using EMPTY local repository. Expectation was that build should succeed in both cases, and both should produce same local repository.

Results:
$ find local-repository/ -type f | wc -l
4442
$ du -sh local-repository/
175M local-repository/
$

Same in both cases.

[cstamas]

IMHO, this looks good

[cstamas]

Run Maven ITs (57a1cf81bb87fbf46bd8189558a10633fe8bf65e) against maven using resolver-1.8.0-SNAPSHOT-BFS (built as above): aside those ITs known to fail due transport-http, rest is OK.

[INFO] 
[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   MavenITmng3652UserAgentHeaderTest>AbstractMavenIntegrationTestCase.runTest:265->testmng3652_AdditionnalHttpHeaderConfiguredInSettings:334 expected:<[Maven Fu]> but was:<[Apache-Maven/4.0.0-alpha-1-SNAPSHOT (Java 11.0.14; Linux 5.13.0-28-generic)]>
[ERROR]   MavenITmng3652UserAgentHeaderTest>AbstractMavenIntegrationTestCase.runTest:265->testmng3652_UserAgentConfiguredInSettings:298 expected:<[Maven Fu]> but was:<[Apache-Maven/4.0.0-alpha-1-SNAPSHOT (Java 11.0.14; Linux 5.13.0-28-generic)]>
[ERROR] Errors: 
[ERROR]   MavenITmng3485OverrideWagonExtensionTest>AbstractMavenIntegrationTestCase.runTest:265->testitMNG3485:48 » Verification
[ERROR]   MavenITmng3599useHttpProxyForWebDAVMk2Test>AbstractMavenIntegrationTestCase.runTest:265->testitUseHttpProxyForWebDAV:226 » Verification
[ERROR]   MavenITmng3600DeploymentModeDefaultsTest>AbstractMavenIntegrationTestCase.runTest:265->testitMNG3600ModesSet:92 » Verification
[ERROR]   MavenITmng3600DeploymentModeDefaultsTest>AbstractMavenIntegrationTestCase.runTest:265->testitMNG3600NoSettings:50 » Verification
[ERROR]   MavenITmng3600DeploymentModeDefaultsTest>AbstractMavenIntegrationTestCase.runTest:265->testitMNG3600ServerDefaults:71 » Verification
[ERROR]   MavenITmng3652UserAgentHeaderTest>AbstractMavenIntegrationTestCase.runTest:265->testmng3652_UnConfiguredDAV:179 » Verification
[ERROR]   MavenITmng4360WebDavSupportTest>AbstractMavenIntegrationTestCase.runTest:265->testitJackrabbitBasedImpl:61->test:140 » Verification
[ERROR]   MavenITmng4360WebDavSupportTest>AbstractMavenIntegrationTestCase.runTest:265->testitSlideBasedImpl:73->test:140 » Verification
[ERROR]   MavenITmng4877DeployUsingPrivateKeyTest>AbstractMavenIntegrationTestCase.runTest:265->testit:56 » Verification
[ERROR]   MavenITmng5175WagonHttpTest>AbstractMavenIntegrationTestCase.runTest:265->testmng5175_ReadTimeOutFromSettings:133 » Verification
[ERROR]   MavenITmng7349RelocationWarningTest>AbstractMavenIntegrationTestCase.runTest:265->testit:48 » Verification
[INFO] 
[ERROR] Tests run: 882, Failures: 2, Errors: 11, Skipped: 0

Errored ITs are either Wagon related (I used maven with transport-http), or unrelated MavenITmng7349RelocationWarningTest (due local repoman).

So I declare ITs OK

[michael-o]

Will take a look next week. Kindly create a new JIRA issue for this and an explanation of your idea.

[caiwei-ebay]

@michael-o
MRESOLVER-240 is fired for this PR. Please check.

[michael-o]

Thank you, assigned. Will pick up next week.