Skip to content

Conversation

ramanathan1504
Copy link
Contributor

#3663 This pull request updates the dependencies in the log4j-layout-template-json-test/pom.xml file to keep the project up to date with the latest Elastic Stack versions and adds a new test dependency. The most important changes are:

Dependency updates:

  • Bumped the elastic.version property from 8.17.3 to 9.1.3 to align with the latest Elastic Stack release.

New test dependency:

  • Added the elasticsearch-rest-client library (version 8.13.4) as a test-scoped dependency to support testing with the Elasticsearch REST client.

Copy link

github-actions bot commented Aug 28, 2025

Job Requested goals Build Tool Version Build Outcome Build Scan®
build-macos-latest clean install 3.9.8 Build Scan PUBLISHED
build-ubuntu-latest clean install 3.9.8 Build Scan PUBLISHED
build-windows-latest clean install 3.9.8 Build Scan PUBLISHED
Generated by gradle/develocity-actions

@ppkarwasz
Copy link
Contributor

Hi @ramanathan1504, thanks for the PR!

After checking the Elasticsearch Java client 9.0.0 release notes:

  • The Java baseline is now JRE 17. Our CI runs tests on JRE 8, so the integration test will fail there even if it passes locally (you can reproduce locally by setting CI=true).
  • The legacy RestClient was deprecated in favor of Rest5Client (based on Apache HttpComponents 5.x, since version 4.x is EOL).

Proposal

  1. Upgrade main to use Rest5Client.

  2. Keep the 2.x branch on Elasticsearch 8.x and update .github/dependabot.yaml to ignore 9.x (and above) proposals.

  3. Add a reminder to drop 8.x when it goes EOL in ~2 years (see the EOL policy) by annotating the test class with JUnit Pioneer:

    @FailAt(
        date = "2027-07-15",
        reason = "Elasticsearch 8.x is no longer supported"
    )

@ramanathan1504
Copy link
Contributor Author

@ppkarwasz
Thank you for the feedback
I will close this PR and create new ones and follow your proposal.

@github-project-automation github-project-automation bot moved this from To triage to Done in Log4j bug tracker Aug 31, 2025
@vy
Copy link
Member

vy commented Sep 1, 2025

@ramanathan1504, please don't close PRs and start a new one each time – it makes it impossible to track the evolution of the eventual PR.

@ppkarwasz
Copy link
Contributor

I agree: the changes to .github/dependabot.yaml must be done on the default branch, which is 2.x, so they can be done in this PR.

The @FailAt annotation also needs to be done in 2.x.

Only the upgrade to Rest5Client needs a separate PR.

@vy
Copy link
Member

vy commented Sep 1, 2025

I think

  1. we're comparing apples with oranges. We use ES client to test a functionality, the fact that it depends on Java 17 should not stop us from using it, in particular, given that we already use Java 17 to compile the sources. That is, we already have Java 17 at our disposal while running tests – saying "Don't use Java 17 because our CI tests run in Java 8" does not make sense to me.
  2. There is only one problematic test: LogstashIT. It is executed via maven-failsafe-plugin configured in the docker profile defined in log4j-layout-template-json-test/pom.xml, it has nothing to do with java8-tests profile in log4j-parent/pom.xml, and the latter forces Java 8.
  3. Using different major versions across 2.x and main will obstruct porting process.

@ppkarwasz, what about the following plan instead?

  1. Use the latest and greatest ES client on 2.x
  2. Update maven-failsafe-plugin in the docker profile in log4j-layout-template-json-test/pom.xml to force Java 17

@ppkarwasz
Copy link
Contributor

Sounds good!

BTW, I was also considering dropping tests on Java 8, since the difference between JRE 8 and 9 is very painful.

@ramanathan1504 ramanathan1504 reopened this Sep 3, 2025
@github-project-automation github-project-automation bot moved this from Done to Waiting for user in Log4j bug tracker Sep 3, 2025
@ramanathan1504
Copy link
Contributor Author

@ppkarwasz @vy
I am reopened the pr, also little confused if anyone of you describe what's needed to do?

@vy
Copy link
Member

vy commented Sep 3, 2025

@ramanathan1504, you can proceed with the following plan:

  1. Use the latest and greatest ES client on 2.x
  2. Update maven-failsafe-plugin in the docker profile in log4j-layout-template-json-test/pom.xml to force Java 17

@ramanathan1504
Copy link
Contributor Author

@vy
ok

@ramanathan1504 ramanathan1504 changed the title Bump elasticsearch-java to version 9.x #3663 Bump elasticsearch-java to version 9.x Sep 6, 2025
@vy vy self-assigned this Sep 6, 2025
@vy vy added the dependencies Related to third party dependency updates or migrations label Sep 6, 2025
Copy link
Contributor

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

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

Hi @ramanathan1504,

Looks good to me, except the two remarks below.

@ppkarwasz
Copy link
Contributor

@ramanathan1504,

You lost some signatures, can you rebase with git rebase -S?

@ramanathan1504
Copy link
Contributor Author

@ppkarwasz
ok i will do that

@ramanathan1504 ramanathan1504 force-pushed the issue-3663 branch 2 times, most recently from 61a2cc9 to d5019f0 Compare September 12, 2025 15:06
@ramanathan1504
Copy link
Contributor Author

@ppkarwasz
commits all are verified

Copy link
Contributor

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! 💯

@ppkarwasz ppkarwasz enabled auto-merge (squash) September 12, 2025 16:40
@ramanathan1504 ramanathan1504 requested a review from vy September 13, 2025 16:28
@ramanathan1504
Copy link
Contributor Author

@vy
Is this right?

recent 8.x version of the modern client library (co.elastic.clients:elasticsearch-java) for the Java code.

the new 9.x version for the Docker images of Elasticsearch and Logstash.

auto-merge was automatically disabled September 21, 2025 08:00

Head branch was pushed to by a user without write access

@vy
Copy link
Member

vy commented Sep 26, 2025

@vy Is this right?

recent 8.x version of the modern client library (co.elastic.clients:elasticsearch-java) for the Java code.

the new 9.x version for the Docker images of Elasticsearch and Logstash.

No, use ES 9 everywhere. We use ES only for integration tests and, as @ppkarwasz stated earlier (I cannot find the commit associated with his message, since you force-pushed changes, but the conversation is still visible), ITs run using Java 17 anyway.

@ramanathan1504 ramanathan1504 force-pushed the issue-3663 branch 2 times, most recently from d71c756 to d985699 Compare September 30, 2025 22:07
@ramanathan1504
Copy link
Contributor Author

ramanathan1504 commented Sep 30, 2025

@vy Is this right?
recent 8.x version of the modern client library (co.elastic.clients:elasticsearch-java) for the Java code.
the new 9.x version for the Docker images of Elasticsearch and Logstash.

No, use ES 9 everywhere. We use ES only for integration tests and, as @ppkarwasz stated earlier (I cannot find the commit associated with his message, since you force-pushed changes, but the conversation is still visible), ITs run using Java 17 anyway.
@vy
am reverted back the LogstashIT to old and bumped the version only

@vy
Copy link
Member

vy commented Oct 3, 2025

@ramanathan1504, after several review rounds, I could not succeed in making you remove redundant dependencies and unrelated changes. I eventually carried them out myself in 561107e. Please be more attentive to these details next time – it will make life easier for both of us.

@vy vy enabled auto-merge (squash) October 3, 2025 13:06
@ramanathan1504
Copy link
Contributor Author

Really sorry for this,i will take care these things in future and i will retrospect this pr and comments..next time will do the good one thanks @vy

@vy vy merged commit 84c7716 into apache:2.x Oct 3, 2025
11 checks passed
@github-project-automation github-project-automation bot moved this from Waiting for user to Done in Log4j bug tracker Oct 3, 2025
@vy
Copy link
Member

vy commented Oct 8, 2025

@ramanathan1504, would you submit a PR porting this to main, please? (Bump the version to 9.1.5 instead.)

@ramanathan1504
Copy link
Contributor Author

@vy sure i will do
thanks

@ramanathan1504 ramanathan1504 deleted the issue-3663 branch October 9, 2025 05:07
ramanathan1504 added a commit to ramanathan1504/logging-log4j2 that referenced this pull request Oct 10, 2025
… methods; update pom.xml for dependency organization

Signed-off-by: Ramanathan <[email protected]>
@ramanathan1504
Copy link
Contributor Author

@vy can you review the pr #3951

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

Labels

dependencies Related to third party dependency updates or migrations

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants