Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat(SystemClock): Added PreciseClock implementation to the system clock class, for the benefit of Java 8 users. #3217

Open
wants to merge 1 commit into
base: 2.x
Choose a base branch
from

Conversation

jaykataria1111
Copy link
Contributor

@jaykataria1111 jaykataria1111 commented Nov 19, 2024

Straight forward change to override the behavior of System Clock, to implement the Precise Clock interface, for the benefit of Java 8 users. The PR is for issue #3198

Checklist

  • Base your changes on 2.x branch if you are targeting Log4j 2; use main otherwise
  • ./mvnw verify succeeds (if it fails due to code formatting issues reported by Spotless, simply run ./mvnw spotless:apply and retry)
  • Non-trivial changes contain an entry file in the src/changelog/.2.x.x directory
  • Tests for the changes are provided
  • Commits are signed (optional, but highly recommended)

@jaykataria1111 jaykataria1111 marked this pull request as ready for review November 19, 2024 08:18
Copy link

github-actions bot commented Nov 22, 2024

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

The build failure is due to the 2.24.2 release being published (BND checks your code against 2.24.2 now, instead of 2.24.1 before).

Please, rebase.

@jvz
Copy link
Member

jvz commented Dec 3, 2024

Code changes look good to me. Can you add a changelog entry for this? You can copy src/changelog/.2.x.x /3121_instant_format.xml as an example of a changed entry.

@ppkarwasz ppkarwasz self-assigned this Dec 4, 2024
@jaykataria1111
Copy link
Contributor Author

jaykataria1111 commented Dec 13, 2024

Hi @jvz , @ppkarwasz There we go did it 📦

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.

@jaykataria1111,

Since the Java 8 and Java 9+ versions of the SystemClock class are identical now, can you remove the Java 9+ version (in log4j-core-java9). The log4j-core-java9 package has a lot of dummy classes, whose purpose is to make the module compilable, they are not packaged. Can you remove those dummy classes too?

/**
* Implementation of the {@code Clock} interface that returns the system time.
* @since 2.25
Copy link
Contributor

Choose a reason for hiding this comment

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

The class itself is older, so we should remove the @since.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good!

Comment on lines +38 to +40
/**
* {@inheritDoc}
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I find {@inheritDoc} a little bit useless, since IDEs will look at the Javadoc of the parent class/implemented interface anyway, if the Javadoc is missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, will remove this too!

@jaykataria1111
Copy link
Contributor Author

@ppkarwasz

Great opportunity for me to learn here:

The log4j-core-java9 package has a lot of dummy classes, whose purpose is to make the module compilable, they are not packaged. Can you remove those dummy classes too

  1. What does it mean to make the module "compatible"?
  2. they are not packaged - How do I identify the classes that are packaged.
  3. How do I identify these dummy classes?

@ppkarwasz
Copy link
Contributor

@jaykataria1111 ,

The log4j-core-java9 package has a lot of dummy classes, whose purpose is to make the module compilable, they are not packaged. Can you remove those dummy classes too

1. What does it mean to make the module "compatible"?

To make it "compilable".

The log4j-core-java9 module is compiled before log4j-core and has no access to the classes of log4j-core.
In order to compile a class like SystemClock we had to add to log4j-core-java9 also the classes it depends upon, e.g., those in the core.time directory.

These are the classes that I call "dummy" classes.

2. they are not packaged - How do I identify the classes that are packaged.

We use the Maven Assembly plugin to package the classes in log4j-core-java9. There is an explicit list of classes to package in the java9.xml descriptor.

3. How do I identify these dummy classes?

They should have a comment near the top that says that they are "dummy" or something equivalent.

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.

3 participants