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
JAPI-110 Add log4j support #202
Conversation
@rpastrana please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jpmcmu looks good, a couple of comments on the utils methods.
Also, let's leave off of this pr anything outside of org.hpccsystems.ws.client.
Those other projects release independently (we'll submit those changes on a different pr)
@@ -64,14 +68,24 @@ public static String parseVersionFromWSDLURL(String wsdlurl) | |||
|
|||
static public void println(PrintStream stream, String message, boolean onlyifverbose, boolean verbosemode ) | |||
{ | |||
if (verbosemode || !onlyifverbose) | |||
stream.println(message); | |||
// if (verbosemode || !onlyifverbose) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's remove the commented out code
} | ||
|
||
static public void print(PrintStream stream, String message, boolean onlyifverbose, boolean verbosemode) | ||
{ | ||
if (verbosemode || !onlyifverbose) | ||
stream.print(message); | ||
// if (verbosemode || !onlyifverbose) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's remove the commented out code here as well
stream.println(message); | ||
// if (verbosemode || !onlyifverbose) | ||
// stream.println(message); | ||
if (onlyifverbose) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (onlyverbose && !verbosemode)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rpastrana Does using the incoming verbosemode still make sense if we are using log4j? Wouldn't we want the user to control that strictly via the log4j log-level settings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, its still a parameter in the methid signature, it'd be odd to ignore it. Unless targeting debug vs error in log4j implies verbose va nonverbose?
if (onlyifverbose) { | ||
log.debug(message); | ||
} else { | ||
log.error(message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this might need to target log.trace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you saying use debug & trace or error & trace?
@@ -64,14 +68,24 @@ public static String parseVersionFromWSDLURL(String wsdlurl) | |||
|
|||
static public void println(PrintStream stream, String message, boolean onlyifverbose, boolean verbosemode ) | |||
{ | |||
if (verbosemode || !onlyifverbose) | |||
stream.println(message); | |||
// if (verbosemode || !onlyifverbose) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's label these 2 methods deprecated. Let's add a new log method log (String message, level(error|debug|trace|etc.) to be more in tune w/ log4j
- Added log4j as a dependency - Added log4j.properties file - Replaced current logging functionality with log4j Signed-off-by: James McMullan <James.McMullan@lexisnexis.com>
- Moved to an older version of log4j that doesnt require newer Java versions - Deprecated Utils.print & Utils.println - Changed default log level in print & println to info / warn - Added log function to Utils for end users to potential use - Reversed logging changes in rdf2hpcc & clienttools
@rpastrana please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jpmcmu looks good
* JAPI-110 Add log4j support - Added log4j as a dependency - Added log4j.properties file - Replaced current logging functionality with log4j Signed-off-by: James McMullan <James.McMullan@lexisnexis.com> * Code review changes - Moved to an older version of log4j that doesnt require newer Java versions - Deprecated Utils.print & Utils.println - Changed default log level in print & println to info / warn - Added log function to Utils for end users to potential use - Reversed logging changes in rdf2hpcc & clienttools
Signed-off-by: James McMullan James.McMullan@lexisnexis.com
Type of change:
Checklist:
Testing: