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

Teach Platform to load via ServiceLoader #12

Closed
wants to merge 1 commit into from
Closed

Teach Platform to load via ServiceLoader #12

wants to merge 1 commit into from

Conversation

cslee00
Copy link
Contributor

@cslee00 cslee00 commented Apr 22, 2018

Allows back-ends to be loaded by dropping in JARs supporting ServiceLoader semantics. Preserves existing behaviour of only allowing a single Platform to be provided

@googlebot
Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@cslee00
Copy link
Contributor Author

cslee00 commented Apr 22, 2018

Signed CLA

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

@ronshapiro
Copy link
Collaborator

I'm not sure that ServiceLoader is what we'd want, because it can have bad performance in certain environments. Perhaps a command line flag to the JVM might serve the same goal without any performance considerations?

/cc @hagbard

@ronshapiro
Copy link
Collaborator

Ah I just saw that @hagbard has an internal PR too have this configurable via system property.

@cslee00
Copy link
Contributor Author

cslee00 commented Apr 22, 2018 via email

ronshapiro pushed a commit that referenced this pull request May 2, 2018
…onfigured via system properties. Also made GooglePlatform no longer subclass DefaultPlatform since that had no real benefit in the design.

I think there's more to do here, since it's really hard to test these classes. However this CL should let open-source stuff (e.g. log4j backend get moving better).

Fixes #12

RELNOTES=Reworking underlying platform configuration to help support log4j backend

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=195079907
@ronshapiro ronshapiro mentioned this pull request May 2, 2018
ronshapiro pushed a commit that referenced this pull request May 2, 2018
…onfigured via system properties. Also made GooglePlatform no longer subclass DefaultPlatform since that had no real benefit in the design.

I think there's more to do here, since it's really hard to test these classes. However this CL should let open-source stuff (e.g. log4j backend get moving better).

Fixes #12

RELNOTES=Reworking underlying platform configuration to help support log4j backend

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=195079907
@cslee00 cslee00 deleted the platform-service-loader branch February 17, 2019 15:38
@Marcono1234
Copy link

Could it please be reconsidered to add support for ServiceLoader as well?
The logic could be:

  • If System property is set, use that class
  • Otherwise try ServiceLoader
  • Otherwise fall back to default

The problems @cslee00 listed are still the case, especially:

maybe limiting in some use cases (inability to set system props in managed environment, for example).

Similarly when a project using Flogger is used as dependency by other projects (with a different logging implementation), for each of them the System property needs to be set, which is rather error-prone (and not always possible). When using ServiceLoader, the project using Flogger can simply add the logging bridge (e.g. Flogger to Log4j 2) and dependent projects won't have to do any special logging configuration (assuming they use the same logging framework).

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.

None yet

4 participants