Skip to content

add OpenRewrite recipes for Apache Commons#16246

Closed
sullis wants to merge 1 commit intoapache:masterfrom
sullis:sean/openwrite-recipes-apache-commons
Closed

add OpenRewrite recipes for Apache Commons#16246
sullis wants to merge 1 commit intoapache:masterfrom
sullis:sean/openwrite-recipes-apache-commons

Conversation

@sullis
Copy link
Copy Markdown
Contributor

@sullis sullis commented Apr 8, 2024

Description

enhance OpenRewrite configuration with the rewrite-apache library

  • enable Apache Commons Collections recipe
  • enable Apache Commons Math recipe

Key changed/added classes in this PR
  • rewrite.yml

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

Copy link
Copy Markdown
Member

@kgyrtkirk kgyrtkirk left a comment

Choose a reason for hiding this comment

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

since there are no usage of these libs - can't we just ban there usages somehow?

I'm not against enabling these rules - but my experience so far is that openrewrite is not a fast piece of equipment to work with - looking at 2 recent runs it seem to me that adding these 2 rules have added 30 extra seconds to its runtime

however...taking a closer look it seems like the runtime fluctuates quite a bit:

What are your experiences? Is it just the "entry cost" of using these recipes are a bit higher...and it doesn't really matter how many recipes it runs?

Comment thread rewrite.yml
type: specs.openrewrite.org/v1beta/recipe
name: org.apache.druid.RewriteRules
recipeList:
- org.openrewrite.apache.commons.math.UpgradeApacheCommonsMath_2_3
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.

it seems to me that there were no usages of commons-math 2 inside the project;
and it seems to me that the math2 library is not even loaded as a dependency

I wonder in the above case why would we need the upgrade recipes for it?

Comment thread rewrite.yml
name: org.apache.druid.RewriteRules
recipeList:
- org.openrewrite.apache.commons.math.UpgradeApacheCommonsMath_2_3
- org.openrewrite.apache.commons.collections.UpgradeApacheCommonsCollections_3_4
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.

I don't see common-collections 3 being used at all - but it was pulled in into the project:

  • opencsv 5.8 -> commons-beanutils:1.9.4 -> commons-collections:3.2.2
  • kafka_test -> commons-validator:commons-validator:jar:1.7:test -> commons-collections:3.2.2

it seems to me that there are only 2 classes being used from commons-collection4

@kgyrtkirk
Copy link
Copy Markdown
Member

in the absense of answers: I'll close this PR as I still don't understand why the need to enable these recipies

@kgyrtkirk kgyrtkirk closed this May 27, 2024
@kgyrtkirk
Copy link
Copy Markdown
Member

I was looking into recent dependabot PRs and it seemed like there are still commons-lang uses ;
which should be upgraded to commons-lang3 - I've done some exploratory work here which have also enabled a rewrite rule for it; then I've realized: you might be interested in doing this - if is that so; please feel free to pick any changes from my branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants