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.

Sign up for GitHub

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

Jump to bottom

JAPI-110 Add log4j support #202

Merged
merged 2 commits into from Sep 26, 2018
Merged

JAPI-110 Add log4j support #202

merged 2 commits into from Sep 26, 2018

Conversation

jpmcmu
Copy link
Contributor

@jpmcmu jpmcmu commented Sep 7, 2018

  • Added log4j as a dependency
  • Added log4j.properties file
  • Replaced current logging functionality with log4j

Signed-off-by: James McMullan James.McMullan@lexisnexis.com

Type of change:

  • This change is a bug fix (non-breaking change which fixes an issue).
  • This change is a new feature (non-breaking change which adds functionality).
  • This change is a breaking change (fix or feature that will cause existing behavior to change).

Checklist:

  • I have created a corresponding JIRA ticket for this submission
  • My code follows the code style of this project.
    • I have applied the Eclipse code-format template provided.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly, or...
    • I have created a JIRA ticket to update the documentation.
    • Any new interfaces or exported functions are appropriately commented.
  • I have read the HPCC Systems CONTRIBUTORS document (https://github.com/hpcc-systems/HPCC-Platform/wiki/Guide-for-contributors).
  • The change has been fully tested:
    • I have performed unit tests to cover my changes.
    • I have performed system test and covered possible regressions and side effects.
    • I have checked that this change does not introduce memory leaks.
    • I have used Valgrind or similar tools to check for potential issues.
  • I have given due consideration to all of the following potential concerns:
    • Scalability
    • Performance
    • Security
    • Thread-safety
    • Premature optimization
    • Existing deployed queries will not be broken
    • This change fixes the problem, not just the symptom
    • The target branch of this pull request is appropriate for such a change.

Testing:

@jpmcmu
Copy link
Contributor Author

jpmcmu commented Sep 7, 2018

@rpastrana please review

rpastrana
rpastrana requested changes Sep 9, 2018
View reviewed changes
Copy link
Member

@rpastrana rpastrana left a 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)

wsclient/src/main/java/org/hpccsystems/ws/client/utils/Utils.java Outdated
@@ -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)
Copy link
Member

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

wsclient/src/main/java/org/hpccsystems/ws/client/utils/Utils.java Outdated
}

static public void print(PrintStream stream, String message, boolean onlyifverbose, boolean verbosemode)
{
if (verbosemode || !onlyifverbose)
stream.print(message);
// if (verbosemode || !onlyifverbose)
Copy link
Member

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

wsclient/src/main/java/org/hpccsystems/ws/client/utils/Utils.java Outdated
stream.println(message);
// if (verbosemode || !onlyifverbose)
// stream.println(message);
if (onlyifverbose) {
Copy link
Member

Choose a reason for hiding this comment

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

if (onlyverbose && !verbosemode)

Copy link
Contributor Author

@jpmcmu jpmcmu Sep 10, 2018 •
edited

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?

Copy link
Member

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?

wsclient/src/main/java/org/hpccsystems/ws/client/utils/Utils.java Outdated
if (onlyifverbose) {
log.debug(message);
} else {
log.error(message);
Copy link
Member

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

Copy link
Contributor Author

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?

wsclient/src/main/java/org/hpccsystems/ws/client/utils/Utils.java Outdated
@@ -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)
Copy link
Member

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

@jpmcmu
JAPI-110 Add log4j support
e79cf13 
- Added log4j as a dependency
- Added log4j.properties file
- Replaced current logging functionality with log4j

Signed-off-by: James McMullan <James.McMullan@lexisnexis.com>
@jpmcmu
Code review changes
83a2aec 
- 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
@jpmcmu
Copy link
Contributor Author

jpmcmu commented Sep 25, 2018

@rpastrana please review

rpastrana
rpastrana approved these changes Sep 26, 2018
View reviewed changes
Copy link
Member

@rpastrana rpastrana left a comment

Choose a reason for hiding this comment

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

@jpmcmu looks good

@rpastrana rpastrana merged commit 39a06a4 into hpcc-systems:master Sep 26, 2018
woodbr01 pushed a commit to woodbr01/HPCC-JAPIs that referenced this pull request Feb 7, 2020
@jpmcmu @rpastrana
JAPI-110 Add log4j support (hpcc-systems#202)
7902f93 
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rpastrana rpastrana rpastrana approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@jpmcmu @rpastrana