FWD » Core Development » Database

While using SQLStatementLogger for multiple databases, I discovered that all the statements are logged to a single file. The culprit for this is SQLStatementLoggerConfiguration.fileLogger which is kept static. fileLogger is assigned in the constructor, so this means that everytime the constructor is called, that class member is reassigned. As a result, the file logger associated for the last called database will be used for all the databases.

The solution is quite simple: just remove the static keyword in SQLStatementLoggerConfiguration. However, this pattern seems to be used also for P2JQueryLogger, in P2JQueryStatisticsConfiguration, fileLogger is also a static variable.

I will make this simple changes and commit them.

Review of 10111a/15978.

The changes are logical. The reason is explained in 1st note above. Good catch!

The commit log is missing the task's #refid.

• (possible NPE): please add the following change in P2JQueryLogger.java: insert at line 133 a return statement after printing the null query notification. Probably this will not be the case, but if it is, a NPE will occur if execution continue;
• (performance issue): in SQLStatementLogger.log(), where SQLStatementLoggerConfiguration.getFileLogger() is called: the sb is ALWAYS computed and only at the end fileLogger != null is tested. This should be done first and sb will not be constructed if true. Also, line 187 should be rewritten with .append() instead of concatenation. Similar for P2JQueryLogger.log(): the file is checked only after calling printResult with sb already constructed.
• (code duplication): the P2JQueryStatisticsConfiguration and SQLStatementLoggerConfiguration are 95% identical.

I have committed in 10111a/15980 the main refactor for having the common logic from P2JQueryStatisticsConfiguration and SQLStatementLoggerConfiguration in a super class: BaseLoggingConfiguration. However, while doing this, I also realised that SQLLoggingDatabaseHelper and P2JLoggingDatabaseHelper can also be refactored by following the same pattern. So, I will continue with this.

Review of 10111a/r15983

First, why in revno: 15983 the files: src/com/goldencode/p2j/persist/BaseLoggingConfiguration.java and src/com/goldencode/p2j/persist/orm/BaseLoggingDatabaseHelper.java were removed? They seem to be used and although I did not build the project, doing so will certainly fail. I think you can use

bzr revert src/com/goldencode/p2j/persist/BaseLoggingConfiguration.java  src/com/goldencode/p2j/persist/orm/BaseLoggingDatabaseHelper.java -r15982

I appreciate the effort to create the new class hierarchies (helpers and configuration) for avoiding the code duplication and specific implementation for each logger type.

• BaseLoggingConfiguration, (missing):
  • this file seems to be in progress. The file header is missing as well as the javadoc for several methods (initializeLoggers(), loadSpecificDirectoryConfiguration() and createDbHelper()). Some indentations (both for parameters and javadoc) should be adjusted;
  • in getConfigurationPath() method please move initalization of globalPath inside else branch and localEnabled into if block (it can be inlined);
• P2JQueryStatisticsConfiguration
  • the access to static getConfiguration() should also synchronised.
  • getLoggingType() does not have the @Override annotation, but it is really identical to its parent so it can be dropped, and loggingType made private;
  • line 13: the secong H entry does not need a new number;
• SQLStatementLoggerConfiguration
  • LoggingType enum is not used (was move to base class)
  • static getConfiguration() should be synchronised;
• BaseLoggingDatabaseHelper, (missing):
  • this file seems to inherit the header from SQLLoggingDatabaseHelper. Please update it with necessary information;
  • multiple indentations are broken compared to original file;
  • dbHelpers field map does not seem to be used;
• P2JLoggingDatabaseHelper
  • please indent the string concatenations at lines 154 and 179;
  • in the static factory getDbHelper() method, the access to dbHelpers should be synchronised.
• SQLLoggingDatabaseHelper
  • please indent the string concatenations at lines 145 and 166;
  • in the static factory getDbHelper() method, the access to dbHelpers should be synchronised.
• P2JQueryLogger.java and SQLStatementLogger.java
  • copyright year needs update

Ovidiu Maxiniuc wrote:

Review of 10111a/r15983

First, why in revno: 15983 the files: src/com/goldencode/p2j/persist/BaseLoggingConfiguration.java and src/com/goldencode/p2j/persist/orm/BaseLoggingDatabaseHelper.java were removed? They seem to be used and although I did not build the project, doing so will certainly fail. I think you can use[...]to get the last revision and continue to work on them linked in bzr (possible to preserve their history in bzr). I did revert them from previous revision and included in the below review.

Sorry, I messed up the local branch a little. I deleted the files at a point and forgot to bzr add them back. I will do that tomorrow and will address the review.

I remembered something worth noting. In my current implementation, I have changed the type of the enabled node for sql-logging and p2j-query-logging. Before it was string, I have changed it to boolean, as I find it more intuitive. This implies changes to directory.xml, however I think these logging tools are only used by us internally, so clients shouldn't be affected. After this will get merged, I will update the wikis accordingly.

I have run a test suite of a large customer application and also tested different options for the logging tools and didn't find any problems.

An important aspect I want to mention again is that I have slightly modified the directory config for these tools. I have changed the type of the enabled node from string to boolean. However, I don't think customers really use these tools, and even if they used it, only a message will be shown in server.log: Node /server/standard/persistence/sql-logging/enabled is expected to have class /meta/class/boolean, but is configured as /meta/class/string. I will just update the corresponding wikis as soon this will be merged.

Ovidiu, do you think there is anything else to be done before merging?

I understand that your test have a good coverage of the affected code.

Related to node type change. If the node is not present in customer directory.xml, nothing happens, but if the node already exists, what will happen when FWD will try to read the node?

Ovidiu Maxiniuc wrote:

but if the node already exists, what will happen when FWD will try to read the node?

If the node already exists and the type hasn't been changed to boolean, this warning entry will be in server.log: 25/07/15 10:05:40.151+0300 | WARNING | com.goldencode.p2j.util.Utils [Utils.executeLookup()] | ThreadName:main, ThreadId:00000003, User:standard | Node /server/standard/persistence/sql-logging/enabled is expected to have class /meta/class/boolean, but is configured as /meta/class/string.. The logging functionality will, of course, be disabled in this case. However, apart from this, the server will work as before, there shouldn't be any functional difference.

OK. If there's just a single warning and the tests showed no regressions then I do not see a reason to delay the merge of the branch into trunk.

I have rebased 10111a to the latest trunk.
Ovidiu, please let me know when can it be merged.

I think so.
Constantin can you queue this branch for merging?

10111a changes should also be ported to 7156e.

I have ported the changes to 7156e as rev. 15764.