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

Enhanced logger #502

Merged
merged 15 commits into from
Jun 1, 2018
Merged

Conversation

aionJoey
Copy link
Contributor

@aionJoey aionJoey commented May 23, 2018

Description

Added additional functionality and configuration to existing logger system;

  • Allows user to enable logger in config.xml file
  • Automatically rollover log files based on size (100MB) and time (daily)
  • Groups archived log files into month folders
  • Allows user to define log folder path
  • Checks for invalid path name input (+ outputs error on terminal)

Fixes Issue # .

Type of change

Insert x into the following checkboxes to confirm (eg. [x]):

  • Bug fix.
  • New feature.
  • Breaking change (a fix or feature that causes existing functionality to not work as expected).
  • Requires documentation update.

Testing

Please describe the tests you used to validate this pull request. Provide any relevant details for test configurations as well as any instructions to reproduce these results.

  • Tested file path and features manually
  • Added test cases for file path output and configurations

Verification

Insert x into the following checkboxes to confirm (eg. [x]):

  • I have self-reviewed my own code and conformed to the style guidelines of this project.
  • New and existing tests pass locally with my changes.
  • I have added tests for my fix or feature.
  • I have made appropriate changes to the corresponding documentation.
  • My code generates no new warnings.
  • Any dependent changes have been made.

@AionJayT AionJayT added this to the v0.2.8 - University Peak milestone May 23, 2018
@AionJayT AionJayT added the enhancement New feature or request label May 23, 2018
Copy link
Contributor

@AlexandraRoatis AlexandraRoatis left a comment

Choose a reason for hiding this comment

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

In addition to the specific comments, please:

  1. update the default config in modBoot/resources/config.xml (with the comments)
  2. ensure the license for each file conforms to https://github.com/aionnetwork/aion/wiki/Aion-License-Header


/** XML - Displays log-path in the config.xml */
xmlWriter.writeCharacters("\t\t");
xmlWriter.writeStartElement("log-path");
Copy link
Contributor

Choose a reason for hiding this comment

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

could you add comments here regarding the meaning of the configuration (similar to the ones in the api and db)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comments into the config.xml file

System.out.println("Logger file path: '" + cfg.getLog().getLogPath() + "'\n");
}

// If commit this out, the config setting will be ignore. all log module been set to "INFO" Level
Copy link
Contributor

Choose a reason for hiding this comment

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

not clear what that comment is mean to say

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed comment


loggerContext = (LoggerContext) LoggerFactory.getILoggerFactory();

/** Toggles file appending configurations */
Copy link
Contributor

Choose a reason for hiding this comment

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

could you switch all the comments that use the java doc notation /** to the comment notation /* throughout all the files of the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the comment notation and added the license headers to relevant files

@@ -104,6 +104,8 @@ public String toXML() {
* Boolean value to allow logger to be toggled ON and OFF
*/
xmlWriter.writeCharacters("\t\t");
xmlWriter.writeCharacters("<!--Enable/Disable logback service; if disabled, output will not be logged -->");
Copy link
Contributor

Choose a reason for hiding this comment

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

you need to use writeComment here like in

xmlWriter.writeCharacters("\r\n\t\t");
xmlWriter.writeComment("Sets the physical location on disk where data will be stored.");
xmlWriter.writeCharacters("\r\n\t\t");
xmlWriter.writeStartElement("path");
xmlWriter.writeCharacters(this.getPath());
xmlWriter.writeEndElement();

@@ -114,6 +116,8 @@ public String toXML() {
* String value to determine the folder path for log files
*/
xmlWriter.writeCharacters("\t\t");
xmlWriter.writeCharacters("<!--Sets the physical location on disk where log files will be stored.-->");
Copy link
Contributor

Choose a reason for hiding this comment

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

same as before, need to use writeComment and remove the comment tags

@AionJayT AionJayT merged commit 8f93329 into aionnetwork:master-pre-merge Jun 1, 2018
@AionJayT AionJayT mentioned this pull request Jun 5, 2018
10 tasks
@AionJayT AionJayT mentioned this pull request Jun 14, 2018
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants