FWD » Core Development » Database

Branch 7416b will be merged into trunk soon (good news). Those changes include the removal of DatabaseStatistics, mainly because it became deprecated and no longer usable since we don't make use of Hibernate anymore. However, with it, there was one interesting feature that was removed: the allowance of collecting query logs/data in a H2 database instead of a basic file.

The point of this task is to "revive" the feature of storing query logs into a H2 database and adapt its usage to our current code base.

I added the necessary modifications to support this feature, changes are now on branch 7960a, rev 15156.

Review of 7960a:

• For P2JLoggingDatabaseHelper construct:
  • Separate the connection from statement execution:
  • If the connection fails, no need to rollback connection or close the statement.
  • If the statement fails, the connection should be roll-backed.
• time_stamp is not a valid name, use timeStamp. Change the javadoc as well.
• In insetP2JQueryProgfiling, move the id null-check before return. An exception will omit that logging.
• closeConnection: if you close a connection, you still need to make it null. Otherwise, you may end up using/closing the same connection all over again.
• In P2JQueryLogger, avoid the import on java.sql.* (for Timestamp). Consider other package not that specific (as sql).

• Make sure that this kind of logging is happening in servers that are not configured of doing such. Doing H2 logging for arbitrary configurations will end up in a big slow-down of production environments. Double-check your changes are not hit on an application that is not configured to do so.

Changes are now on branch 7960a, rev 15157.
Regarding your concern: Make sure that this kind of logging is not happening in servers that are not configured of doing such.
Initially i mimicked the logic writing to file: if no database configuration was present in directory.xml i was using default values. Now you need to specify that database output is enabled as shown bellow:

  <node class="container" name="output-to-db">
    <node class="boolean" name="enabled">
      <node-attribute name="value" value="TRUE"/>
    </node>
    <node class="string" name="db-name">
      <node-attribute name="value" value="p2j_%db_%as"/>
    </node>
    <node class="string" name="db-user">
      <node-attribute name="value" value="user"/>
    </node>
    <node class="string" name="db-pass">
      <node-attribute name="value" value="pass"/>
    </node>
  </node>

If /output-to-db/enabled is not set to true, the logging will only happen in files. The rest of the attributes are not mandatory, if not present we use default values.

• Small issues with P2JLoggingDatabaseHelper:
  • history entry Module is not the same as the class name
  • lacking copyright notice
  • brace after try should be on a new line
• There are places where the stackTrace is generated only if isStackStracingEnabled is set. Otherwise is null. Please check if having stackTrace set to null is a good state (not causing NPE).
• Overall, I am OK with 7960a; please reiterate the changes a second time.

Rebased 7960a to latest trunk. It is now at rev. 15350.

• Please attempt to check a large application without this setting and with this setting + different combinations.
• Smoke testing is enough: no obvious crashes, decent outputs to be provided.

Addressed code review and committed on 7960a rev. 15351.

I looked into the file closing issue. The problem is here:
In P2JQueryStatisticsConfiguration:

fileHandler = new FileHandler(formattedFilePath);
fileHandler.setLevel(Level.ALL);
fileHandler.setFormatter(LOG_FORMATTER);

FileHandler fileHandler = config.getFileHandler();
LogRecord lr = new LogRecord(Level.WARNING, result);
fileHandler.publish(lr);
fileHandler.flush();

Ioana-Cristina Prioteasa wrote:

I could add a server hook and if profiling was enable to close the file.

I think this is a viable solution. CentralLogger comes with its configurations for log levels, I'm not sure should apply to the query loggers. If the common configurations are not an issue, we can look into how to extend it with a new type that accepts a different file config.

The existing code was using the CentralLogger format and WARNING log level, so I think it's would be great if the CentralLogger is involved. Besides, having the option to use a logger with a configurable file seems like a great addition. The implementation is almost done, I will commit by the end of the day.

Committed changes to 7960a rev.15352. Added a concrete version of CentralLogger that allows dynamic file path configuration and used that for logging the queries to files.
Galya, please review.

Ioana-Cristina Prioteasa wrote:

Committed changes to 7960a rev.15352. Added a concrete version of CentralLogger that allows dynamic file path configuration and used that for logging the queries to files.
Galya, please review.

• Remove the condition writing to the pre-init buffer in CentralLoggerFile.publish. It's about directory configs still not being resolved, but CentralLoggerFile doesn't depend on them.
• CentralLoggerFile.ConfigsBuilder seems unnecessary only for 3 configs. I think the defaults can be applied directly in CentralLoggerFile.initialize instead of creating the extra config objects. Anyways, not critical, do as you like.
• Clean up the imports in SQLStatementLogger, SQLStatementLoggerConfiguration, P2JQueryStatisticsConfiguration, P2JQueryLogger. Check if the packages can be imported in CentralLoggerFile instead of the individual classes.

CentralLoggerFile doesn't depend on the core CentralLogger initialization / configuration, so it should be fine.

Addressed code review.
I retested the scenario where filepath is null or empty, as I was concerned about potential issues. The server didn't crash and severe logs were correctly written, which is the expected and desired behavior.
Changes are on 7960a rev. 15353. Please review.

Made a small error in CentralLoggerFile at fileCount and fileLimit conditions. Resolved it and committed the fix 7960a rev. 15354.

Ioana-Cristina Prioteasa wrote:

Made a small error in CentralLoggerFile at fileCount and fileLimit conditions. Resolved it and committed the fix 7960a rev. 15354.

It's good.

• return javadoc doesn't need to have the type; only the description of the returned value
• please replace the //ignore with a LOG statement. Also, LOG should allow message and cause as parameters. Avoid doing e.getMessage, because you won't have the stack trace printed in that case. Prefer to use LOG.warning("message", e)
• change the order of methods to sort public first and private last

• P2JLoggingDatabaseHelper seems to have only one connection that is not synchronized. Also, dbHelpers seems to be a plain HashMap with no concurrent control.
• the same goes for SQLLoggingDatabaseHelper

Lets get this tested according to the plan (if something left).

Please attempt to check a large application without this setting and with this setting + different combinations.
Smoke testing is enough: no obvious crashes, decent outputs to be provided.or testing:

To test concurrency, you can use "parallel" CLI (or maybe other tool) with the FWD client to spawn more client doing work on parallel (maybe 1000 queries each). You should check that 10 users will eventually generate exactly 10000 queries in the logging database (SQL and P2J query) without errors.

Branch 7960a merged to trunk as rev. 15394 and archived.