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

PUBDEV-7073 extended logging possibilities #4109

Merged
merged 1 commit into from
Dec 9, 2019

Conversation

koniecsveta
Copy link
Contributor

when creating a hex.genmodel.easy.EasyPredictModelWrapper enableContributions parameter is false by default already.

h2o-logger/build.gradle Outdated Show resolved Hide resolved
h2o-logger/build.gradle Outdated Show resolved Hide resolved
h2o-logger/build.gradle Outdated Show resolved Hide resolved
@koniecsveta koniecsveta force-pushed the PUBDEV-7073-exteded-logging-possibilities branch 2 times, most recently from 7106068 to 541393b Compare November 28, 2019 22:21
h2o-logger/src/main/java/water/logging/Logger.java Outdated Show resolved Hide resolved
scripts/jenkins/Makefile.jenkins Outdated Show resolved Hide resolved
h2o-logger/src/test/java/water/logging/LoggerTest.java Outdated Show resolved Hide resolved
h2o-logger/src/test/java/water/logging/LoggerTest.java Outdated Show resolved Hide resolved
@koniecsveta koniecsveta force-pushed the PUBDEV-7073-exteded-logging-possibilities branch 2 times, most recently from 7b9e13a to efefd54 Compare December 2, 2019 20:22
@koniecsveta koniecsveta force-pushed the PUBDEV-7073-exteded-logging-possibilities branch from efefd54 to d209db4 Compare December 3, 2019 14:10
* @return Slf4JLogger if SLF4J is on the classpath, ConsoleLogger if not
*/
public Logger getCustomLogger(Class<?> clazz) {
if (isSlf4JAvailable) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we make the decision just once?

IMHO better for the MOJO use case

Copy link
Contributor

Choose a reason for hiding this comment

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

this is done once per class static-init so one if() is not a big issue I think

@koniecsveta koniecsveta force-pushed the PUBDEV-7073-exteded-logging-possibilities branch from d209db4 to 83ba402 Compare December 5, 2019 20:22
@koniecsveta koniecsveta changed the base branch from rel-yau to master December 5, 2019 20:23
@michalkurka
Copy link
Contributor

@koniecsveta looks good, please change the base branch to master

also I realized: we should also delegate to water.util.Log if the MOJO code is running inside H2O. Parts of genmodel are used by h2o runtime - in that case the logging should go to water.util.Log instead of system out/err.

@koniecsveta @honzasterba WDYT?

* @return Slf4JLogger if SLF4J is on the classpath, ConsoleLogger if not
*/
public Logger getCustomLogger(Class<?> clazz) {
if (isSlf4JAvailable) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is done once per class static-init so one if() is not a big issue I think

private static final String DEFAULT_CLASS_TO_CHECK = "org.slf4j.LoggerFactory";
private static final LoggerFactory INSTANCE = new LoggerFactory(DEFAULT_CLASS_TO_CHECK);

private final String slf4jClassName;
Copy link
Contributor

Choose a reason for hiding this comment

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

we could extend this with waterClassName and look for water.util.Log if slf4j is not found

Copy link
Contributor Author

Choose a reason for hiding this comment

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

making it this way we get circular dependency between modules with different language levels assigned: genmodel(7)->core(8)->logger(7)->genmodel(7).

@koniecsveta koniecsveta force-pushed the PUBDEV-7073-exteded-logging-possibilities branch from ea54ec0 to 0e00a5e Compare December 6, 2019 18:11
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