Skip to content

[fix][build] Client modules should be built with Java 8#19991

Merged
eolivelli merged 3 commits into
apache:masterfrom
tisonkun:admin-java8
Apr 3, 2023
Merged

[fix][build] Client modules should be built with Java 8#19991
eolivelli merged 3 commits into
apache:masterfrom
tisonkun:admin-java8

Conversation

@tisonkun

@tisonkun tisonkun commented Apr 1, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

This fixes #19982.

Motivation

Client modules are promised to be compatible with Java 8.

Modifications

Add --releaase 8 for these two remaining client modules.

Verifying this change

Locally tested.

Before:

file target/classes/org/apache/pulsar/client/impl/auth/AuthenticationAthenz.class
target/classes/org/apache/pulsar/client/impl/auth/AuthenticationAthenz.class: compiled Java class data, version 61.0

After:

file target/classes/org/apache/pulsar/client/impl/auth/AuthenticationAthenz.class 
target/classes/org/apache/pulsar/client/impl/auth/AuthenticationAthenz.class: compiled Java class data, version 52.0 (Java 1.8)

I don't know if we can add a CI step to ensure it.

Add enforce plugin to prevent regressions.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

@tisonkun
[fix][build] Client modules should be built with Java 8
4e9a1be 
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun tisonkun self-assigned this Apr 1, 2023
@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Apr 1, 2023
@tisonkun tisonkun mentioned this pull request Apr 1, 2023
Closed
2 tasks
lhotari
lhotari requested changes Apr 1, 2023
View reviewed changes
Comment thread pom.xml Outdated
eolivelli
eolivelli requested changes Apr 1, 2023
View reviewed changes

@eolivelli eolivelli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The broker modules should be compiled for jdk17

I agree that client must stay on 8.

@tisonkun

tisonkun commented Apr 1, 2023

Copy link
Copy Markdown
Member Author

Hi @lhotari & @eolivelli!

The changeset for updating the broker compiler version is for profile integration-test-java8. But I agree I can revert the change as it won't hurt.

@tisonkun
leave pulsar.broker.compiler.release and pulsar.client.compiler.relea…
aa2c7a0 
…se blank for profile integration-test-java8

Signed-off-by: tison <wander4096@gmail.com>
@tisonkun

tisonkun commented Apr 1, 2023

Copy link
Copy Markdown
Member Author

Updated. Please give another look.

@tisonkun tisonkun requested review from eolivelli and lhotari April 1, 2023 13:49
@tisonkun
enforce JDK 8 for client-auth-xxx modules
29f99a8 
Signed-off-by: tison <wander4096@gmail.com>
lhotari
lhotari approved these changes Apr 1, 2023
View reviewed changes

@lhotari lhotari left a comment

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.

LGTM

@codecov-commenter

codecov-commenter commented Apr 1, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.82%. Comparing base (68c10ee) to head (29f99a8).
⚠️ Report is 2673 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #19991      +/-   ##
============================================
- Coverage     72.89%   72.82%   -0.08%     
+ Complexity    31619    31592      -27     
============================================
  Files          1861     1861              
  Lines        137356   137362       +6     
  Branches      15117    15118       +1     
============================================
- Hits         100131   100039      -92     
- Misses        29271    29366      +95     
- Partials       7954     7957       +3
Flag Coverage Δ
inttests 24.44% <ø> (+<0.01%) ⬆️
systests 25.00% <ø> (-0.04%) ⬇️
unittests 72.11% <ø> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 76 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mattisonchao mattisonchao added the release/blocker Indicate the PR or issue that should block the release until it gets resolved label Apr 3, 2023
eolivelli
eolivelli approved these changes Apr 3, 2023
View reviewed changes

@eolivelli eolivelli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perfect

+1

@eolivelli eolivelli merged commit 5ef3a21 into apache:master Apr 3, 2023
@tisonkun tisonkun deleted the admin-java8 branch April 3, 2023 08:01
@tisonkun

tisonkun commented Apr 3, 2023

Copy link
Copy Markdown
Member Author

Hi @Technoboy-! This can be a release blocker to 2.11.1. Since you're the RM for this release, I'd like to let you know this patch and ask if you can cherry-pick it to branch-2.11 and include it in 2.11.1.

Technoboy- pushed a commit that referenced this pull request Apr 3, 2023
@tisonkun @Technoboy-
[fix][build] Client modules should be built with Java 8 (#19991)
f3b69ef
@Technoboy- Technoboy- added cherry-picked/branch-2.11 and removed release/blocker Indicate the PR or issue that should block the release until it gets resolved labels Apr 3, 2023
@Technoboy-

Technoboy- commented Apr 3, 2023

Copy link
Copy Markdown
Contributor

Hi @Technoboy-! This can be a release blocker to 2.11.1. Since you're the RM for this release, I'd like to let you know this patch and ask if you can cherry-pick it to branch-2.11 and include it in 2.11.1.

Thanks, cherry-picked to branch-2.11.

@heesung-sohn heesung-sohn mentioned this pull request Apr 6, 2023
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@merlimat merlimat merlimat approved these changes

@lhotari lhotari lhotari approved these changes

@eolivelli eolivelli eolivelli approved these changes

@RobertIndie RobertIndie RobertIndie approved these changes

@Technoboy- Technoboy- Awaiting requested review from Technoboy-

Assignees

@tisonkun tisonkun

Labels

cherry-picked/branch-2.11 doc-not-needed Your PR changes do not impact docs ready-to-test release/2.11.1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug] Pulsar2-11.0 pulsar-client AuthenticationSasl.class can not be used in java8

8 participants

@tisonkun @codecov-commenter @Technoboy- @merlimat @lhotari @eolivelli @RobertIndie @mattisonchao