Skip to content

[MRESOLVER-248] Make DF and BF collector implementations coexist#161

Merged
cstamas merged 10 commits intomasterfrom
MRESOLVER-248-collector-bf-df-coexists
Apr 8, 2022
Merged

[MRESOLVER-248] Make DF and BF collector implementations coexist#161
cstamas merged 10 commits intomasterfrom
MRESOLVER-248-collector-bf-df-coexists

Conversation

@cstamas
Copy link
Copy Markdown
Member

@cstamas cstamas commented Apr 4, 2022

Revive replaced DF collector, and make them coexists. Default one is "old" DF, while new BF may be activated on demand (based on session config).

This is simple "reshuffle" PR:

  • moves BF related classes to bf package
  • (re)introduces DF related classes to df package
  • implements DefaultDependencyCollector indirecting component
  • opens up (makes public) required methods on helper classes

So, mostly is simple, just "move around". I'd suggest we merge this as is (if OK), and have another iteration as I think there may be some duplications and maybe other refactoring possibilities.


https://issues.apache.org/jira/browse/MRESOLVER-248

@cstamas cstamas requested a review from michael-o April 4, 2022 15:29
@cstamas cstamas self-assigned this Apr 4, 2022
@cstamas
Copy link
Copy Markdown
Member Author

cstamas commented Apr 4, 2022

@caiwei-ebay ping

cstamas added 2 commits April 4, 2022 20:29
It is really internal thing of BF skipper, no need for
iface + 2 separate classes.

Expose and adjust tests.
@cstamas
Copy link
Copy Markdown
Member Author

cstamas commented Apr 4, 2022

I think I collapsed/deduped mostly what was obvious...

Comment thread maven-resolver-impl/src/main/java/org/eclipse/aether/impl/guice/AetherModule.java Outdated
@cstamas
Copy link
Copy Markdown
Member Author

cstamas commented Apr 5, 2022

General remark: the convention to name package as "internal" or "impl" should automatically imply this is INTERNAL thing (AFAIK, some OSGi tooling and Takari life-cycle even enforce this). In this case we deal with "internal.impl" 😄 so IMHO it should be really really clear to developer that this is internal implementation... Am just saying this is something generally used, but agree, will update javadoc

@cstamas cstamas requested a review from michael-o April 5, 2022 07:09
@cstamas
Copy link
Copy Markdown
Member Author

cstamas commented Apr 7, 2022

@michael-o any blocker here?

@cstamas
Copy link
Copy Markdown
Member Author

cstamas commented Apr 7, 2022

@caiwei-ebay all looking good here? Any proposal to change/add?

@michael-o
Copy link
Copy Markdown
Member

Will review today/tomorrow.

@caiwei-ebay
Copy link
Copy Markdown
Contributor

@caiwei-ebay all looking good here? Any proposal to change/add?

Looks good to me. Thanks.

@kwin
Copy link
Copy Markdown
Member

kwin commented Apr 8, 2022

@cstamas
Copy link
Copy Markdown
Member Author

cstamas commented Apr 8, 2022

Right, doco changes are missing (2 new config keys), will add them

Comment thread src/site/markdown/configuration.md Outdated
Copy link
Copy Markdown
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

Almost done. Will run tests.

if ( logger.isDebugEnabled() )
{
stats.put( "BfDependencyCollector.collectTime", time2 - time1 );
stats.put( "BfDependencyCollector.transformTime", time3 - time2 );
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Stupid question: If you already have the logger class name in the ouput, if necessary, why print it again here?

Copy link
Copy Markdown
Member Author

@cstamas cstamas Apr 8, 2022

Choose a reason for hiding this comment

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

am unsure does Maven uses "standard" layout w/ classes as log names... is it? I'd just don't bother 😄

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, you can easily enable with SLF4J Simple, if necessary

@michael-o michael-o self-requested a review April 8, 2022 11:10
Copy link
Copy Markdown
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

Looks good now. Please squash and change title: "Make DF and BF collector implementations coexist"

@cstamas cstamas changed the title [MRESOLVER-248] Make BF and DF collectors coexists [MRESOLVER-248] Make BF and DF collectors coexist Apr 8, 2022
@cstamas cstamas changed the title [MRESOLVER-248] Make BF and DF collectors coexist [MRESOLVER-248] Make DF and BF collector implementations coexist Apr 8, 2022
@cstamas cstamas merged commit 4b9255b into master Apr 8, 2022
@cstamas cstamas deleted the MRESOLVER-248-collector-bf-df-coexists branch April 8, 2022 11:16
@jira-importer
Copy link
Copy Markdown

Resolve #1402

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.

5 participants